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

fix(a11y): Replace Collapse and Button with Accordion components in PlanningConstraints #2966

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Apr 2, 2024

What does this PR do?

  • Replaces the combination of Button and Collapse components with an Accordion component
  • Moves some Accordion theming to the MUI theme so that there's more consistency with our other accordions (currently just ResultReason)
  • No intentional visual changes
    • Apart from one!
    • The colour for the Caret and hover is now fixed and not based on primary colour - I wasn't sure if this was intentional in the first place, or a hangover from using a Button.
    • If you could take a look at this one please @ianjon3s and confirm that would be much appreciated! 🙌
Before After
Screenshot image image
Link Storybook (Prod) Storybook (Pizza)

Motivation

@DafyddLlyr DafyddLlyr changed the base branch from main to dp/planning-constrains-multiple-lists April 2, 2024 08:27
shouldForwardProp: (prop) => prop !== "category",
})<StyledConstraintProps>(({ theme, category }) => ({
const StyledAccordion = styled(Accordion, {
shouldForwardProp: (prop) => !["category", "metadata", "content", "data"].includes(prop as string),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verbose, but it resolves this issue -

image

Copy link

github-actions bot commented Apr 2, 2024

Removed vultr server and associated DNS entries

@DafyddLlyr DafyddLlyr requested a review from a team April 2, 2024 10:07
@DafyddLlyr DafyddLlyr marked this pull request as ready for review April 2, 2024 10:08
@DafyddLlyr
Copy link
Contributor Author

List structure [A]

Copy link
Contributor

@ianjon3s ianjon3s left a comment

Choose a reason for hiding this comment

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

A few small comments on the diffs relating to ID usage and styling parity.

Really good to see this updated to the appropriate accordion component!

@DafyddLlyr
Copy link
Contributor Author

Thanks @ianjon3s - some great catches there. All three issues should be fixed by 11c24aa

@ianjon3s ianjon3s self-requested a review April 2, 2024 14:37
@DafyddLlyr DafyddLlyr merged commit 5218c64 into dp/planning-constrains-multiple-lists Apr 2, 2024
10 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/planning-constraints-accordions-a11y branch April 2, 2024 17:05
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.

2 participants