Skip to content

Conversation

manuq
Copy link
Collaborator

@manuq manuq commented Jun 27, 2023

Resolves #686

manuq added 6 commits June 27, 2023 13:11
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).
@manuq manuq marked this pull request as ready for review June 27, 2023 20:08
@GeorgesStavracas
Copy link
Contributor

I've found this situation when testing this branch:

Captura de tela de 2023-06-27 16-34-24

Joana hasn't seen this yet, but I suspect the desired behavior in this case is to left align the grid instead of center it.

Copy link
Contributor

@GeorgesStavracas GeorgesStavracas left a 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.

@manuq
Copy link
Collaborator Author

manuq commented Jun 27, 2023

@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
@manuq manuq force-pushed the new-carousel-cards branch from 5a418ef to 9e80711 Compare June 27, 2023 22:34
Copy link
Contributor

@jprvita jprvita left a 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.

Comment on lines +67 to +68
EkSlidableGridNew,
EkSlidableCardGridNew,
Copy link
Contributor

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.

@manuq manuq marked this pull request as draft June 30, 2023 20:15
@manuq
Copy link
Collaborator Author

manuq commented Jun 30, 2023

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.

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.

@jprvita
Copy link
Contributor

jprvita commented Jul 3, 2023

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.

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 new suffix works, but if we are taking more than a couple of days I would still use a more meaningful name, just in case we end up switching priorities and leaving this potential for confusion behind for longer than we wanted (aka tech debt).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement new carousel of cards
3 participants