-
Notifications
You must be signed in to change notification settings - Fork 4
New carousel cards #685
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
base: master
Are you sure you want to change the base?
New carousel cards #685
Conversation
The upcoming component needs the gutter value in Javascript.
Using the vue-ssr-carousel dependency. These are interchangeable with the previous slider, except the composition is a bit different because the new one occupies the full page width. So it shouldn't be inside a (non fluid) b-container.
This will make it easy to migrate from the current "slidable" (which is default).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch series is fantastic, it was super easy to review. Thank you for that.
I had to learn what <b-container fluid>
means, and AFAIU one would pass fluid
to make the container have 100% of the width.
The only thing that I found was the centralized grid I described above. Now that I'm testing it more, I wonder if it should scroll when scrolling horizontally with touchpads too.
@GeorgesStavracas thanks for testing! You are absolutely right about the bad centering. I realize that we don't need a carousel component at all if there is no pagination (less-or-equal than 4 cards in a big screen). So I appended a commit that uses a simple (responsive) grid in that case. |
If there is no need to paginate, just use a responsive grid. Also update current grid configuration to match: - 4 cards in large - 2 cards in medium - 1 card in small
5a418ef
to
9e80711
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not have time to test this yet, but read through the code. It LGTM, with a small nitpick + bikeshedding opportunity on the name of the new component.
EkSlidableGridNew, | ||
EkSlidableCardGridNew, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we choose a better name for these components? EkSlidableGrid
vs EkSlidableGridNew
has zero meaning for someone who was not around when they were created. I tried to think of some suggestions and the best I could come up with was EkInertialSlidableGrid
, but I'm not sure how good that is.
The new one is meant to replace the current one. I don't see any reason to keep the old component once we are happy with the new implementation. My idea was to temporarely keep it as a separate component (suffixed "new") for comparing them side-by-side. |
I thought we would want to keep both components. If we are replacing the old one immediately, the |
Resolves #686