Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(form-v2): tiles component #2380

Merged
merged 15 commits into from
Aug 24, 2021
Merged

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Jul 13, 2021

Problem

We require a tile component to display info compactly. This PR adds the Tile component as specified in the Figma design here.

Closes #2024

Solution

The tile component is a wrapper component - i.e, its styles are decided by us and the people who use it are not given (as much) control over the underlying styling of the component itself. The general approach towards designing this component will be laid out below

Tag
Allow users to pass in tag as a component rather than specifying the tag's styles as props for the user to pass in. This gives the user more control of how they want the tag to look and makes the tiles component simpler also.

Description
Instead of having a specific description prop (like tag), the more general children prop was chosen. This is because the Tile component is more of a wrapper component, and it makes more sense to have children as the description rather than separately defining description and then allowing a children prop, which does not make sense.

Variant
As variants are determined based on whether there is a description, we can determine this automatically instead of adding extra props. A type is defined for safety and clarity.

Questions for reviewers

  1. How much stylistic power should be exposed on the component? The gauge i used here was that the figma did not seem to allow for much stylistic deviation and i thought that the tiles component was to be used 'as is' rather than having extensive customization done hence, did not expose any style props.
  2. Is the approach taken towards designing this component logical?

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible

Screenshots
This is available in the generated storybook snapshot

TODO
add more stories

@seaerchin seaerchin marked this pull request as draft July 13, 2021 06:08
@seaerchin seaerchin force-pushed the form-v2/tiles-component branch from 9969c5a to e250521 Compare July 13, 2021 08:49
@seaerchin seaerchin changed the base branch from form-v2/develop to form-v2/tag-component July 13, 2021 08:55
@seaerchin seaerchin force-pushed the form-v2/tiles-component branch from e250521 to 6a122b7 Compare July 13, 2021 08:57
@seaerchin seaerchin marked this pull request as ready for review July 13, 2021 08:57
@seaerchin seaerchin requested a review from karrui July 13, 2021 08:57
Copy link
Contributor

@chowyiyin chowyiyin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text spills out of the tile when its too long
Screenshot 2021-07-14 at 11 06 18 AM
Maybe we can add overflow-wrap to the theme?

return (
// Ref passed into the component as a whole so that it can be focused
<Box sx={styles.container} ref={ref}>
<VStack spacing="1rem" alignItems="flex-start">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be putting all styles into the theme?

Copy link
Contributor Author

@seaerchin seaerchin Jul 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree with putting parts of the component into the theme; however, i think this is more of a utility component (thus, not a part) and should be left as is, cos otherwise our themes run the risk of being overloaded @__@

i'll experiment with using VStack as the outer component and removing teh Box so that we can put it in themes!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can always use the _first selector to only apply the styling to the first child

frontend/src/components/Tile/Tile.tsx Outdated Show resolved Hide resolved
@seaerchin
Copy link
Contributor Author

The text spills out of the tile when its too long
Screenshot 2021-07-14 at 11 06 18 AM
Maybe we can add overflow-wrap to the theme?

pepehands ;--; will do!!

Comment on lines 55 to 60
<Box>
<Text sx={styles.title} as="h4">
{title}
</Text>
<Text sx={styles.subtitle}>{subtitle}</Text>
</Box>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can export the Description, Title and Subtitle wrappers as a subcomponent so outside users can build their own tag with their own styling (if needed)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could i clarify on this point - i actually decided against doing this for the reasons stated in the PR write-up and i wanted to know when this is preferred (or is this all the time b/c we r building our own design system)

Copy link
Contributor

@karrui karrui Jul 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the Tile component is more of a wrapper component

Precisely what you said in your PR description. So you expose Tile.Tile, Tile.Title, Title.Description, in addition to the main Tile component that uses all of those things precisely so users can modify any part of the component if they want instead of building another one, since all it is is a wrapper containing various styling

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha - to confirm this: wrapper components should export their wrapped components because it helps people to style components if needed; but wrt this, we shouldn't export a Description component directly right?

primarily because this is a wrapper and doesn't contribute anything of value beyond the chakra tags so we should export the components that are different (i took this approach)

frontend/src/theme/components/Tile.ts Outdated Show resolved Hide resolved
frontend/src/components/Tile/Tile.tsx Outdated Show resolved Hide resolved
@seaerchin
Copy link
Contributor Author

seaerchin commented Jul 15, 2021

The text spills out of the tile when its too long
Screenshot 2021-07-14 at 11 06 18 AM
Maybe we can add overflow-wrap to the theme?

pepehands ;--; will do!!

@chowyiyin this is cursed... i tried what's documented here as well as reading mdn docs for overflow-wrap.

i'm not too sure what's causing chakra to not wrap text actually and any help would be appreciated ;--;

The main issue is because of maxWidth being set which, if removed, will not have overflow because the container stretches to fit.

@seaerchin seaerchin force-pushed the form-v2/tiles-component branch 2 times, most recently from f43e15f to 98999b0 Compare July 15, 2021 06:50
@seaerchin seaerchin requested review from karrui and chowyiyin July 26, 2021 05:31
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nits, fix the hover styling on active state and we are gtg!

frontend/src/theme/components/Tile.ts Show resolved Hide resolved
frontend/src/theme/components/Tile.ts Show resolved Hide resolved
frontend/src/components/Tile/Tile.stories.tsx Outdated Show resolved Hide resolved
frontend/src/components/Tile/Tile.stories.tsx Outdated Show resolved Hide resolved
@seaerchin seaerchin requested a review from pearlyong July 29, 2021 06:18
Base automatically changed from form-v2/tag-component to form-v2/develop August 24, 2021 04:53
@karrui karrui force-pushed the form-v2/tiles-component branch from ea6cb8d to 9e0b287 Compare August 24, 2021 05:01
@karrui karrui merged commit 33d5124 into form-v2/develop Aug 24, 2021
@karrui karrui deleted the form-v2/tiles-component branch August 24, 2021 08:26
@justynoh justynoh mentioned this pull request Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants