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: Ratings #1654

Merged
merged 6 commits into from Jun 14, 2023
Merged

feat: Ratings #1654

merged 6 commits into from Jun 14, 2023

Conversation

Mahmoud-zino
Copy link
Sponsor Contributor

Linked Issue

Closes #1028

Description

Added RatingBar component.

Notes:

  1. As Discussed on Discord, for now, the RatingBar is simply a Presentation component.
  2. I need some Ideas for the usage slot in the docs 🙏 .

Changsets

feat: Added svelte RatingBar component.

Checklist

Please read and apply all contribution requirements.

  • This PR targets the dev branch (NEVER master)
  • Documentation reflects all relevant changes
  • Branch is prefixed with: docs/, feat/, chore/, bugfix/
  • Ensure Svelte and Typescript linting is current - run pnpm check
  • Ensure Prettier linting is current - run pnpm format
  • All test cases are passing - run pnpm test
  • Includes a changeset (if relevant; see above)

@changeset-bot
Copy link

changeset-bot bot commented Jun 9, 2023

🦋 Changeset detected

Latest commit: 7a20ad8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
skeleton.dev Minor
@skeletonlabs/skeleton Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jun 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
skeleton-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 14, 2023 5:22am

@Mahmoud-zino Mahmoud-zino changed the title added Rating-Bar component feat: RatingBar Jun 9, 2023
@endigo9740
Copy link
Contributor

endigo9740 commented Jun 9, 2023

@Mahmoud-zino I'll do a deeper review soon, but real quick:

  1. Rename the component <Ratings /> and the route /ratings
  2. Change the default SVG fill color to fill-on-primary-token for a more neutral and accessible style
  3. Numeric slot names make me nervous, so let's do empty | half | full

I'll review the code and such in a follow up! But looks to be a great start!

@Mahmoud-zino Mahmoud-zino changed the title feat: RatingBar feat: Ratings Jun 10, 2023
@jamess-ais
Copy link

Whoops sorry, was viewing and saw there was a review button and was curious if I had the perms... turns out I do!

Ignore my approval!

@endigo9740
Copy link
Contributor

endigo9740 commented Jun 12, 2023

@Mahmoud-zino I've pushed a few changes and improvements to the doc and component itself. This includes:

  • Flushed out the doc with more examples
  • Added interactive binding for the current/max values - which is fun to play with!
  • Updated the local imported icons file to be a bit more organized, include circles, moved attribution
  • SVG image size is now responsive, so we can lead by example
  • Update the component to include fill and text style props, with default values.
  • Minor comment updates to match conventions

Unless you see anything else I'm happy with this and ready to merge!

@endigo9740
Copy link
Contributor

endigo9740 commented Jun 12, 2023

@Mahmoud-zino couple follow up thoughts...

  1. Should we maybe go ahead and wrap each icon with a span (or button) and add regionIcon classes so users can add styles wrapping each icon? Users could do something like regionIcon="cursor-pointer" if they want these to be interactive
  2. Per interactivity - maybe when clicking one of these divs we dispatch an event that displays the Each loop index value? They'll already know how many max items there are, so setting the current value will just be a matter of checking the index provided, like:
    • index 0 = 1st icon selected
    • index 4 = 5th icon (out of 10)
    • index 9 = 10th icon (out of 10)

These two small changes would allow users to make this interactive on click. Not sure how half states are set - but that would be at the user's discretion.

@Mahmoud-zino
Copy link
Sponsor Contributor Author

@endigo9740

Some Notes

  1. I wrapped the icons with buttons because adding on:click on span will enrage A11y, the down side is without interactivity the users will still get the cursor-pointer and the ring around the button.
  2. I added a User Interactivity section to the docs with only the code on how to make the component user interactive. Should I add more details?

@endigo9740
Copy link
Contributor

endigo9740 commented Jun 13, 2023

@Mahmoud-zino this is nearly perfect, one more thing with the buttons - make sure you always apply a default type, in this case type="button". If you don't the buttons will default to type="submit", which can interfere with form functionality. We might also consider abstracting this to a prop so it's user configurable.

If you can get that in by tomorrow I'll review and we'll likely go ahead and merge. Super excited to include this, as it's a long awaited feature request that's sat idle for quite some time! Great job!

@Mahmoud-zino
Copy link
Sponsor Contributor Author

@endigo9740
Yeah, totally correct, I forgot about it.
I changed the type to button but didn't add a prop because I don't think you would ever consider adding the rating click as a submit for your form.

@endigo9740 endigo9740 merged commit b359bc7 into skeletonlabs:dev Jun 14, 2023
3 checks passed
@endigo9740
Copy link
Contributor

Great job on this one!

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.

Rating Component
3 participants