-
-
Notifications
You must be signed in to change notification settings - Fork 331
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
Make Nav.Tile id mandatory and update examples #3097
base: next
Are you sure you want to change the base?
Make Nav.Tile id mandatory and update examples #3097
Conversation
🦋 Changeset detectedLatest commit: 2aa638d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@phamduylong is attempting to deploy a commit to the Skeleton Labs Team on Vercel. A member of the Team first needs to authorize it. |
{/snippet} | ||
{#snippet tiles()} | ||
<Navigation.Tile id="0" labelExpanded="Browse Files" href={hrefExample}> | ||
<Navigation.Tile id="1" labelExpanded="Browse Files" href={hrefExample}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phamduylong this is one of the only unique components we introduced that doesn't use Zag (intentionally). Now that I look at this, I wonder if rather than making ID mandatory, we utilize useId()
to set a default automatically.
We didn't have access to useId()
in Svelte when this component was built - that was introduced only when we integrated Zag. React has it built in as part of the library.
I think that'll provide a better experience - the ID will always be set, but users can still overwrite with a custom ID as they wish. Which would mean id
would be optional again. Once less setting users have to deal with.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for me. A bit less work for users, I wouldn't like to specify these if they aren't practically useful for my project either. I'll implement that.
Linked Issue
Closes #3085
Description
Make
id
mandatory on Nav.Tile instancesChangsets
Instructions: Changesets automate our changelog. If you modify files in
/packages
, runpnpm changeset
in the root of the monorepo, follow the prompts, then commit the markdown file. Changes that add features should beminor
while chores and bugfixes should bepatch
. Please prefix the changeset message withfeat:
,bugfix:
orchore:
.Checklist
Please read and apply all contribution requirements.
dev
branch (NEVERmaster
)docs/
,feat/
,chore/
,bugfix/
pnpm ci:check
pnpm format
pnpm test