Next: ratings

[Mahmoud-zino]

Linked Issue

Closes #2392

[changeset-bot]

⚠️ No Changeset found

Latest commit: 4744e02

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[vercel]

Someone is attempting to deploy a commit to the Skeleton Labs Team on Vercel.

A member of the Team first needs to authorize it.

[vercel]

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

1 Ignored Deployment

Name	Status	Preview	Updated (UTC)
skeleton-docs	⬜️ Ignored (Inspect)	Visit Preview	May 23, 2024 7:58pm

[endigo9740]

@Mahmoud-zino just took a quick look at this. I won't provide a full review as I know it's still a draft - but make sure you're not hardcoding SVGs within components if it can be avoid. While we're taking an opinionated stance on Lucide, that doesn't mean we're keen to embed icons from it.

[Mahmoud-zino]

@Mahmoud-zino just took a quick look at this. I won't provide a full review as I know it's still a draft - but make sure you're not hardcoding SVGs within components if it can be avoid. While we're taking an opinionated stance on Lucide, that doesn't mean we're keen to embed icons from it.

This is just a fallback svg (star since it is the most common one for rating) but if you want to use another approach I am totally ok with it, just let me know what would you prefer.

[endigo9740]

Yeah, no fallback SVG. Users will be required to add their own icons. Placeholder text is probably fine though.

[Mahmoud-zino]

@endigo9740 Please ignore the fallback SVG at the moment, I want to get your opinion on the fractional approach before continuing the implementation.
so this approach is using clip-path to hide parts of the full icon to display the empty icon under it, this will enable us to support fractions fully with ease, you can look at the example svelte file to see it in action...
please let me know if I should continue this approach or you have something else in mind

[endigo9740]

@Mahmoud-zino clip-path is a great way to handle this! I like it. Let's do it!

[endigo9740]

@Mahmoud-zino as part of prepping for tomorrow's release I'm doing a pass through all pending PRs. Let me know a status update on this one. What's remaining and ETA for delivery. If you're bogged down, let me or the team know and someone can likely jump in and pick this up! (Hugo, etc)

[Mahmoud-zino]

@endigo9740 Functionality and design of the component is now implemented and ready for review 👍

[endigo9740]

@Mahmoud-zino I'm so sorry, I've been swamped with the Floating UI Svelte stuff this week and completely missed this.

I'll aim to review in the next day or two, but could you do me a huge favor - could you install Lucide for the Svelte and React packages as dev dependencies (via --save-dev flag). Then go ahead and swap out any raw SVGs with icons from those?

• Svelte: https://lucide.dev/guide/packages/lucide-svelte
• React: https://lucide.dev/guide/packages/lucide-react

Remember, we will NEVER use Lucide in our components, but it's perfectly valid to pass to our components in the SvelteKit and Vite/React sandboxes. I've been meaning to do this for a while, and it'll be helpful for your component here.

Please and thanks sir!

[Mahmoud-zino]

@endigo9740 Sorry for taking so long....
I have updated the icons in all example components, and while switching to lucide, I found some minor issues in the Ratings component, these should be fixed now 😉

[endigo9740]

Thanks @Mahmoud-zino this is on my agenda to review today, assuming no further interruptions!

[endigo9740]

@Mahmoud-zino here's my review. A few things of note:

• I LOVE the fractional units. That's so much cleaner.
• I'm kind of so-so on the hover states, that may be overkill and adding extra complexity
• Keep the interaction though. Clicking to set state is nice.
• If we can reduce complexity enough, I'd like us to consider inserting input state so this can be used in forms!

Let me know if I can clarify any points raised below. I'd suggest we go through another round of review before proceeding to tests and React components.

[endigo9740]

@Mahmoud-zino I've bumped this back to a draft to help better indicate when it's ready for review again!

[Mahmoud-zino]

@endigo9740 I have left you a couple of comments back that needs some clarifications 😄 Thanks.
Also about the hover, I removed it but I am really thinking we should keep it, it is a lot easier to select fractions when you are getting immediate feedback.

If we can reduce complexity enough, I'd like us to consider inserting input state so this can be used in forms!

Do you mean adding a prop so users can change the type of the buttons to submit a form? if not, could you please provide more details?

[endigo9740]

@Mahmoud-zino this is still on my list to review. Just tied up with the auto-doc stuff atm.

Do you mean adding a prop so users can change the type of the buttons to submit a form? if not, could you please provide more details?

I believe buttons and inputs cannot have their type dynamically set. Svelte will complain about this. In this case I think we should rely on users to handle submit themselves and keep the buttons at type="button"

[endigo9740]

@Mahmoud-zino hopefully you don't murder me for taking the initiative here, but I've completed a PR review and a bit of a refactor to the component. This streamlines a lot of the complexity and uses much less code for the same results.

Notable changes include:

• The local embedded snippet is gone; code has been moved within the each block
• The <svelte:element> in gone in favor of a hardcoded button
• The button hover state is disabled by pointer-events-none when interactive is false.
• I've refactored the styles to confirm to the base/dynamic/classes pattern for less redundancy (see reference)
• I've change the semantics for a few of the prop names
• Fixed an issue where snippets names internally/externally did not match

My hope here is that by slimming down the complexity, it should make it easier to focus on adding keyboard interactions. That would be the final piece before this ready to create documentation and port to React. I'll set this back to a draft state until we reach one of those milestones.

Let me know what you think!