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

Add support for sl-dropdown in the default slot of sl-breadcrumb-item #2015

Merged

Conversation

schilchSICKAG
Copy link
Contributor

This PR adds the ability to add <sl-dropdown> in the default slot of a <sl-breadcrumb-item> as requested and discussed in #2011, this feature.

The following changes where made to make this work:

The render call does not rely on the href attribute alone to render. Instead, a new state renderType is created that may hold one of the following values: 'button' | 'link' | 'drop-down'. As before, it defaults to button, making all original examples work.

A slotChange listener is bound to all default slots that checks for availability of a slotted <sl-dropdown>. If one is found, the render type is set to drop-down. In all other cases, the logic from the original rendering applies.

I also have added tests and updated the documentation. However, I am unsure how to document this. Should we leave the initial example as is or create a new one (which I did in this PR)?

Looking forward to your suggestions.

Copy link

vercel bot commented May 14, 2024

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

Name Status Preview Updated (UTC)
shoelace ✅ Ready (Inspect) Visit Preview Jul 2, 2024 6:46am

@schilchSICKAG schilchSICKAG changed the title Add support for dropdowns in the default slot Add support for sl-dropdown in the default slot of sl-breadcrumb-item May 14, 2024
Copy link
Collaborator

@KonnorRogers KonnorRogers left a comment

Choose a reason for hiding this comment

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

This looks good to me! (And makes me want hrefs for menu items even more!)

@claviska I dont see any issue here.

Perhaps a different problem for a different day, but we may want to expose some way to style these breadcrumbs differently based on the renderType.

Before you could check the href attribute, now its a little murkier because the labels dont have parts that correspond, and the renderType doesn't get reflected.

schilchSICKAG and others added 3 commits July 2, 2024 08:34
Co-authored-by: Konnor Rogers <konnor5456@gmail.com>
Co-authored-by: Konnor Rogers <konnor5456@gmail.com>
Co-authored-by: Konnor Rogers <konnor5456@gmail.com>
@schilchSICKAG
Copy link
Contributor Author

Hi @KonnorRogers,

thank you for your input. I just committed your suggestions, seems fine for me.

Perhaps a different problem for a different day, but we may want to expose some way to style these breadcrumbs differently based on the renderType.

You are right, we should think about this. Currently, you could use a slotted selector, but thats not nice at all. Conditionally setting the part is also kind of ugly as you will not be able to target all elements with a single selector anymore. We could also reflect the renderType, so it gets easier to target via CSS. Got some other ideas?

@claviska
Copy link
Member

claviska commented Jul 2, 2024

Perhaps a different problem for a different day, but we may want to expose some way to style these breadcrumbs differently based on the renderType.

Would this be for user's custom styles? If so, can the :has selector help here?

@schilchSICKAG
Copy link
Contributor Author

It would be a possibility for sure.

@claviska claviska merged commit 2704f72 into shoelace-style:next Aug 5, 2024
2 checks passed
@claviska
Copy link
Member

claviska commented Aug 5, 2024

Thank you for the solid PR!

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