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

Ced 1746 es carousel #1512

Merged
merged 19 commits into from
Sep 9, 2024
Merged

Ced 1746 es carousel #1512

merged 19 commits into from
Sep 9, 2024

Conversation

hroth1994
Copy link
Contributor

@hroth1994 hroth1994 commented Sep 4, 2024

πŸ”— Linked issue

https://energysage.atlassian.net/browse/CED-1746

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Putting a PR up to get initial feedback on the new EsCarousel component that uses PrimeVue's Carousel. I'm still working on getting a few things working (see Feedback section).

πŸ₯Ό Testing

Tested locally on http://localhost:8500/molecules/carousel - rotated through the carousel on different viewports and pausing autoplay

🧐 Feedback Requested / Focus Areas

A few known issues:

  • At the end of the carousel, it restarts rather than continuing one by one
  • When stopping autoplay, the carousel restarts
  • Refreshing shows a different order of items - seems to be a known issue with circular

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.
  • I have documented testing approach

@hroth1994 hroth1994 marked this pull request as ready for review September 6, 2024 12:40
@tomleo
Copy link
Member

tomleo commented Sep 6, 2024

πŸ‘€

Copy link
Member

@tomleo tomleo left a comment

Choose a reason for hiding this comment

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

Looking good! A few comments inline.

Also I think a Carousel might be better categorized as an Organism given it's composed of other molecules

es-ds-components/components/es-carousel.vue Outdated Show resolved Hide resolved
es-ds-components/components/es-carousel.vue Show resolved Hide resolved
es-ds-docs/pages/molecules/carousel.vue Outdated Show resolved Hide resolved
Copy link
Collaborator

@nathanielwarner nathanielwarner left a comment

Choose a reason for hiding this comment

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

My one suggestion at this point is to add primevue/carousel to the optimizeDeps in the nuxt config in es-ds-components, to stop the annoying auto-refresh while loading

Copy link
Collaborator

@nathanielwarner nathanielwarner left a comment

Choose a reason for hiding this comment

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

I like the cats! Other than merge conflicts, I think the issues you identified are the only ones I see. Do you think it's gonna be possible to address them with the PrimeVue carousel? I guess it's a product question as to whether it's acceptable, but they don't affect our current carousels.

Copy link
Member

@tomleo tomleo left a comment

Choose a reason for hiding this comment

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

Looks good, only question is where nuxt/image should live (in the layer or in the docs)

es-ds-docs/nuxt.config.ts Show resolved Hide resolved
@hroth1994
Copy link
Contributor Author

I like the cats! Other than merge conflicts, I think the issues you identified are the only ones I see. Do you think it's gonna be possible to address them with the PrimeVue carousel? I guess it's a product question as to whether it's acceptable, but they don't affect our current carousels.

I think it could be possible with more investigation, and I'm also curious to see if it's improved in the next version.

@hroth1994 hroth1994 merged commit 01a3795 into esds-3.0-vue3-primevue Sep 9, 2024
1 check passed
@hroth1994 hroth1994 deleted the ced-1746-es-carousel branch September 9, 2024 13:40
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.

4 participants