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-1822 EsRating follow-up TODOs #1531

Merged
merged 15 commits into from
Oct 4, 2024

Conversation

tomleo
Copy link
Member

@tomleo tomleo commented Sep 18, 2024

πŸ”— Linked issue

❓ 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

  • resolving double alerts by using @focus instead of @change

πŸ₯Ό Testing

  • manual

🧐 Feedback Requested / Focus Areas

  • Fading the scale transition after a "click" is still an open TODO

πŸ“ Checklist

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

Copy link

swarmia bot commented Sep 18, 2024

βœ… Β Linked to Task CED-1822 Β· EsRating follow-up TODOs
➑️  Part of Epic CED-1339 · Build Vue 3 design system [MC]

@tomleo tomleo requested a review from hroth1994 September 18, 2024 15:43
Copy link
Contributor

@hroth1994 hroth1994 left a comment

Choose a reason for hiding this comment

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

Mentioned this on Slack - the change event is still triggering twice, you just can't tell anymore without the alert. I do like the new experience in your PR, but I'm not sure removing the alert is ideal since it's a good way of observing that PrimeVue has this issue

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

@tomleo can you add the PR description and address formatting errors?

@tomleo tomleo changed the title Ratings Tweaks CED-1822 EsRating follow-up TODOs Oct 1, 2024
@tomleo tomleo requested a review from hroth1994 October 1, 2024 19:16
Copy link
Contributor

@hroth1994 hroth1994 left a comment

Choose a reason for hiding this comment

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

This is looking good, a few comments and it needs a PR description. I would include in the description what is being fixed (updates to the docs page and resolving double alerts by using @Focus instead of @change) and what still needs to be changed (star transform persisting after click)

es-ds-docs/pages/molecules/rating.vue Outdated Show resolved Hide resolved
es-ds-components/components/es-rating.vue Outdated Show resolved Hide resolved
@tomleo tomleo requested a review from hroth1994 October 3, 2024 16:53
Copy link
Contributor

@hroth1994 hroth1994 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 more thoughts:

es-ds-docs/pages/molecules/rating.vue Outdated Show resolved Hide resolved
es-ds-components/components/es-rating.vue Outdated Show resolved Hide resolved
@tomleo tomleo requested a review from hroth1994 October 3, 2024 18:05
@tomleo tomleo requested a review from hroth1994 October 4, 2024 13:31
Copy link
Contributor

@hroth1994 hroth1994 left a comment

Choose a reason for hiding this comment

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

LGTM! 🌟

@tomleo tomleo merged commit 4311416 into esds-3.0-vue3-primevue Oct 4, 2024
1 check passed
@tomleo tomleo deleted the ced-1822-more-es-rating-double-alert branch October 4, 2024 14:03
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.

3 participants