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

Accordion caret classesControlCaret is repeated on SVG element #2586

Closed
Keagel opened this issue Apr 3, 2024 · 5 comments · Fixed by #2597
Closed

Accordion caret classesControlCaret is repeated on SVG element #2586

Keagel opened this issue Apr 3, 2024 · 5 comments · Fixed by #2597
Labels
bug Something isn't working
Milestone

Comments

@Keagel
Copy link

Keagel commented Apr 3, 2024

Current Behavior

Currently, the classesControlCaret class is being added to both the SVG element and its parent, but the one on the SVG element isn't interpolated. The behavior is unchanged as the caret still shows and rotates as it should.

Reference:

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 448 512" class={classesControlCaret}>

Result:

CleanShot 2024-04-03 at 11 12 11@2x

Expected Behavior

No response

Steps To Reproduce

No response

Link to Reproduction / Stackblitz

No response

More Information

No response

@Keagel Keagel added the bug Something isn't working label Apr 3, 2024
@endigo9740 endigo9740 added this to the v2.0 milestone Apr 3, 2024
@salisshuaibu11
Copy link
Contributor

@endigo9740 i would like to take this

@salisshuaibu11
Copy link
Contributor

salisshuaibu11 commented Apr 5, 2024

@endigo9740, I've reviewed the svgCaretIcon. Despite correcting the interpolation, the class remains undefined even though "classesControlCaret" is present. The solution was to embed the SVG directly into the HTML without passing it as a variable and utilizing the @html special tags.

caretIcon

Considering that we're utilizing the "svgCaretIcon" constant in three different locations, is there a more optimal solution available?

@salisshuaibu11
Copy link
Contributor

@endigo9740, incorporating the dollar sign label resolved the issue, although I'm uncertain if this is the most efficient approach.

$: svgCaretIcon = svg

@endigo9740
Copy link
Contributor

@salisshuaibu11 thanks for diving into this. I'd say leave this one to me. I need to dive in an review the full context before I can provide an answer to how we should approach this. And by that time I can probably implement the adjustment directly just as quickly. I'll take all information above into consideration when I get to that point though!

@endigo9740
Copy link
Contributor

endigo9740 commented Apr 5, 2024

@Keagel @salisshuaibu11 after reviewing the source directly I see the issue here. It looks like the SVG was abstracted to a template literal to cleanup the template a bit.

The class={classesControlCaret} carried through, even though that should have been removed. You can see the new home for those styles is now applied to a wrapping div elements here:

<div class="accordion-summary-caret {classesControlCaret}">{@html svgCaretIcon}</div>

If for some reason you need access to to style the SVG element itself, this is already possible using this Tailwind syntax:
https://www.skeleton.dev/docs/styling#tailwinds-arbitrary-variants

For example:

<AccordionItem classesControlCaret="[&>svg]:fill-red-500">...</AccordionItem>

Alternatively, the caret region is actually a Svelte slot, which means you can replace the open and close icons with custom SVGs. Then adjust the styles as desired.
https://www.skeleton.dev/components/accordions#custom-icons

That said, I'll go ahead and submit a quick change to remove the duplicate class on the SVG template literal now to close this out though. This should help avoid further confusion in the future. Expect this to go out as part of the next release of Skeleton v2.

@endigo9740 endigo9740 linked a pull request Apr 5, 2024 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants