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

Rename LayoutCenter to Center and CardList to Grid #146

Merged
merged 2 commits into from
Sep 11, 2020
Merged

Conversation

adamkudrna
Copy link
Member

  • Turn CardList into a more generic Grid
  • Make LayoutCenter more generic: call it simply Center and stop it from taking up 100 % of viewport height

Note for the Center layout: It's now necessary to set the desired height on the parent of the Center layout, eg. when you're using Login screen component which is expected to spread over entire viewport.

Closes #143.

@adamkudrna adamkudrna added the BC Breaking change label Sep 3, 2020
@adamkudrna adamkudrna self-assigned this Sep 3, 2020
@adamkudrna adamkudrna added this to the v0.34.0 milestone Sep 3, 2020
Copy link
Contributor

@bedrich-schindler bedrich-schindler left a comment

Choose a reason for hiding this comment

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

I am unhappy with changing CardList into Grid. As user of the library, I would not be able to simply use it. Before, it was crystal-clear and easy to understand. After change, it would probably cause lot of pain to me (IMHO also to other developers) because I will need to somehow know what grid accepts and how it works.

In other libraries, Grid represents components that matches with CSS Grid. This component does not do that, it does not accept those props.

I think that CardList was correct naming. This Grid component is very stupid, it does not accepts props like Grid component in other libraries, so it cannot be called Grid in RUI.

I do no like change of CenterLayout as well. Center component seems like horizontal-alignment-only component to me.

@adamkudrna
Copy link
Member Author

@bedrich-schindler Thank you for the feedback! Both changes are for better reusability and less specific components. The idea behind it all is as follows.

Grid

It's not just Cards what can be laid out with this component — technically, it can be anything like images, product cards etc. And sice we don't need to support IE, the only reason to restrict its name to the Card use case is also gone — the maximum width defined on the grid itself is sufficient.

Yes, now it may be mistaken for the usual grid component that exists in other UI libraries. The “usual grid” also has a different way of usage: you would need cells or grid items to wrap around the content you want to put in the grid. That's because how the grid is built: with flexbox. However, that's not our case. We want a next-generation grid component using the right tool for the job: the grid layout. Imagine extending the Grid component in the future like this:

<Grid items="3" size="1fr">
  <div></div>
  <div></div>
  <div></div>
</Grid>

You see? Custom column size, number of columns, and no grid item component! 🎊

It could go further with non-repetitive layouts like this:

<Grid columns="minmax(200px, 300px) 1fr">
  <div></div>
  <div></div>
</Grid>

Center

The former LayoutCenter component was only useful for whole-screen centered layouts. But from my experience, there are more situations where I'd appreciate horizontal and vertical centering without the 100vh stuff. Therefore the change towards a better reusable component for the small price of defining the height yourself. And again, it could be extended like this:

<Center height="100vh">
  <div></div>
</Center>

@bedrich-schindler
Copy link
Contributor

bedrich-schindler commented Sep 11, 2020

@adamkudrna Thanks for response. I just wanna tell you that I understand purpose of those changes.

With explanation I agree now with Center component.

I also agree with Grid component although it seems misleading to me. If it is named Grid, I would expect it to have props like items, size, ..., available for this component, simply to match with logic of other UI libraries. If me (a user) sees Grid component, I expect it to have all these useful props like other libraries do. I think I can be useful to have at least basic props like items, size, ..., to be more generic.

We do not have to do it now, but if it is named Grid, I would consider creating issue that adds those props to Grid. And do it as soon as it gets - it means no later than version 1.0 is out.

@adamkudrna
Copy link
Member Author

adamkudrna commented Sep 11, 2020

@bedrich-schindler

We do not have to do it now, but if it is named Grid, I would consider creating issue that adds those props to Grid. And do it as soon as it gets - it means no later than version 1.0 is out.

Yes! That's what I think, too.

…p it from taking up 100 % of viewport height (#143)

It's now necessary to set the desired height on the parent of the `Center` layout,
eg. when you're using `Login` screen component which is expected to spread over
entire viewport.
@adamkudrna adamkudrna merged commit 5b3ebde into master Sep 11, 2020
@adamkudrna adamkudrna deleted the feature/143 branch September 11, 2020 16:06
@adamkudrna adamkudrna changed the title Make LayoutCenter and CardList more generic Rename LayoutCenter to Center and CardList to Grid Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify names of layout components
2 participants