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(display-list): add display list component - FRONT-2342 #2214

Merged
merged 29 commits into from
Oct 12, 2021

Conversation

emeryro
Copy link
Contributor

@emeryro emeryro commented Sep 23, 2021

preview:

specs:

Two templates have been created:

  • Display list, used to display a full list of elements; number of columns is a parameter (1 column = vertical display)
  • Display list item, containing the data, and placed in a display list

@github-actions
Copy link

github-actions bot commented Sep 23, 2021

Copy link
Contributor

@planctus planctus left a comment

Choose a reason for hiding this comment

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

  • Wow, i was not aware of this "request"...it looks to me like it would be the way to build "layouts" using the "content item" as a component that we still don't have, or we could say that the display-list-item template in this pr is basically that

  • The display-list template then should be the one creating the different rendering markup for the "vertical" and the "horizontal" variants of this "meta component".
    In this case we are just using instead to cover the vertical variant to build a simple list in a wrapper while all the examples for the horizontal variant are built only using the item template with all the surrounding markup created in the story so that those lists are not actually an implementation of the display-list component.

  • I find it also very confusing to associate the variant (horizontal or vertical) to the list, i think that is a property of the item, in case, while the list should only consider the number of columns and build the right markup for it.

  • I haven't looked at the specs, but the option of the square image should maybe only be associated with the "horizontal" variant, but it's not clear the reason why, more than just altering the proportions of the image (as written in the control) it is also resizing it to 100px.
    I would also expect the title to support a "link", not sure if there is any specification about this in the specs, though.

  • We had the idea of building this kind of "layout builder", we even created a template for that in ecl-twig but it was more ambitious, meant to dinamycally include the components based on a parameter, which was not working in twing, unfortunately.

  • Not saying we should reproduce that, but i think this pr is basically introducing a "component", which is the display-list-item, while the "layout" template is currently too limited.

@emeryro
Copy link
Contributor Author

emeryro commented Oct 5, 2021

yes, I also considered using custom css to display horizontal version, instead of using the grid. That's a shame, because we would have to duplicate part of the grid code into this component, but that's probably the best way to handle it...
In any case, I think that it makes sense to have the variant (horizontal, vertical) on the display list component, as it will be the one handling the layout

The square image are indeed defined with a fixed size, for some reasons. I will ask for the possibility to have links, that's a good point.

@emeryro
Copy link
Contributor Author

emeryro commented Oct 5, 2021

ok, I've been thinking about this again, and here is what I propose:

at list level

  • parameter to set the number of columns (from 1 to 4). We would have classes like .ecl-list-illustration--col-2
  • css for the display of columns handled directly in the component (maybe with a display: grid)
  • no variant "horizontal" or "vertical" here

at item level

  • parameter to have "horizontal" or "vertical" items (so with images on left or on top)

Copy link
Contributor

@planctus planctus left a comment

Choose a reason for hiding this comment

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

It's ok i think, just in the website i guess we mixed up data and we have examples with the horizontal "variant" and the zebra.
Another small improvement could be to add in the controls "categories" (content and icons) the information that they only refer to the first item.

@planctus planctus merged commit 06326d5 into v3-dev Oct 12, 2021
@planctus planctus deleted the FRONT-2342-display-list branch October 12, 2021 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants