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

<rh-accordion>: color context demo bug/misrepresentation #2151

Open
zeroedin opened this issue Jan 30, 2025 · 4 comments
Open

<rh-accordion>: color context demo bug/misrepresentation #2151

zeroedin opened this issue Jan 30, 2025 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@zeroedin
Copy link
Collaborator

zeroedin commented Jan 30, 2025

In the color-context demo for accordion, we are placing a color-palette attribute on the rh-accordion-header however it is not a color-context provider so it should not have this attribute.

The sibling rh-accordion-panel, however, is a color context provider. But I believe this is a mistake. It is my understanding of the design that we wouldn't want a specific panel to be different than the rest of the accordion, much less the header that controls it. I relate this to that same decision in tabs where the panel isn't a provider and shouldn't be separated in color from the tabs that control it.

@coreyvickery @marionnegp thoughts?

Here is the demo code:

<rh-accordion-header expanded color-palette="lightest">Forced Palette</rh-accordion-header>
<rh-accordion-panel id="nested" color-palette="lightest">
<label> No matter the parent context, this panel should always be
<rh-context-picker target="nested" value="lightest"></rh-context-picker>
</label>
<rh-cta href="#">Call To Action</rh-cta>
</rh-accordion-panel>

This came up when I worked with the context PR and corrected it there. I meant to write this issue up. @Nouveau caught that API change and reminded me I needed to bring this forward as a possible bug/misrepresentation.

Here is the corrected demo code:

<rh-accordion color-palette="lightest" id="nested">
<rh-accordion-header expanded>Forced Palette</rh-accordion-header>
<rh-accordion-panel>
<label> No matter the parent context, this panel should always be
<rh-context-picker target="nested" value="lightest"></rh-context-picker>
</label>
<rh-cta href="#">Call To Action</rh-cta>
</rh-accordion-panel>

And the commit that changes the accordion panel to no longer be a context provider on that PR:

28d5253

If it is true that the accordion panel should not be a provider, we should fix that in advance of the context PR changes.

@zeroedin zeroedin added the bug Something isn't working label Jan 30, 2025
@zeroedin zeroedin added this to the 2025/Q1 — Cubone release milestone Jan 30, 2025
@zeroedin zeroedin self-assigned this Jan 30, 2025
@coreyvickery
Copy link
Collaborator

@zeroedin I don't think header or panel should be providers. @marionnegp Agree?

@marionnegp
Copy link
Collaborator

Agreed. The whole accordion should use the same color palette.

Out of curiosity, if an accordion with color-palette: lightest is nested in an accordion with color-palette: darkest, does the nested accordion still show the darkest color palette?

@zeroedin
Copy link
Collaborator Author

zeroedin commented Jan 31, 2025

Out of curiosity, if an accordion with color-palette: lightest is nested in an accordion with color-palette: darkest, does the nested accordion still show the darkest color palette?

Any component that has color-palette sets that context for itself and its children. Think of it like a line in the sand.

A dark accordion parent can have a child accordion that sets its own context to light. Case in point is the "forced palette" portion of the color-context demo for accordion: https://deploy-preview-2138--red-hat-design-system.netlify.app/elements/accordion/demo/color-context/

The change I'm suggesting in this issue is that <rh-accordion-header> and <rh-accordion-panel> themselves should not be providers. But their parent <rh-accordion> which contains them should, and keep it's children consistent.

The current bug is both documentation whereas rh-accordion-header by code is not a provider but is shown in the demo as carrying a color-palette attr, and the fact that rh-accordion-panel IS a provider, but that should have only been on it's parent to keep consistent color throughout the accordion header/panel combos.

@bennypowers
Copy link
Member

@zeroedin agreed, let's take design's lead on this one.

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
Status: Backlog
Development

No branches or pull requests

4 participants