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 accordion style #138

Merged
merged 6 commits into from
Dec 21, 2023
Merged

Add accordion style #138

merged 6 commits into from
Dec 21, 2023

Conversation

P-Gill97
Copy link
Contributor

No description provided.

@P-Gill97 P-Gill97 self-assigned this Dec 18, 2023
@P-Gill97 P-Gill97 requested a review from a team December 18, 2023 19:05
src/lib/styles.tsx Outdated Show resolved Hide resolved
src/lib/styles.tsx Outdated Show resolved Hide resolved
src/styles/content.scss Show resolved Hide resolved
}

.os-raise-accordion {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't sure where to put this comment, but I thought I'd flag something I noticed when navigating via keyboard: I see red bottom borders on FF (I would screenshot, but when I go to do it they go away 🙃 ). On Chrome they seem to be black. Maybe those are defaults and there is some styling we need to specify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When navigating the accordion by keyboard on firefox I was unable to reproduce the red bottom border. I am using tab and enter to navigate the accordion. Are you using a different method?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 -- No, that's what I'm doing. Maybe I can screen share when we chat later today. Perhaps it is specific to my environment / build of FF.

Copy link
Contributor

Choose a reason for hiding this comment

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

@P-Gill97 : Just as a follow up from our live convo, I confirmed that on my Mac I see the blue vs red when tabbing across the accordion sections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rnathuji
If we add this CSS below it should fix this problem. Although, should we be enforcing what the user's accessibility preferences are on their browser? I am thinking about a scenario where the user may want the tab focus border to be red or any other color. 🤔

.os-raise-accordion-header:focus {
  outline: 1px solid blue;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting article, I did not expect the focus lore to be this complex 😆. I think I'll ask @madisonschear for her preference when shes back.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 -- I see. Playing around a bit, I notice that in general the focus colors when navigating via keyboard vary across my systems (Linux vs Mac) on other items like math, interactives, etc. Given that, I think if we want to tweak these for consistency, it's probably something we need to look into holistically. I vote we leave this alone so we're consistently inconsistent for the time being 😅 .

Copy link
Contributor

Choose a reason for hiding this comment

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

@P-Gill97 - Yeah, good call as far as getting input from UX. I do think that if we want to create CSS to make things consistent, it's probably a separate issue we need to spin up so we can identify / address multiple items that I'm finding can vary.

Copy link
Contributor

@rnathuji rnathuji left a comment

Choose a reason for hiding this comment

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

This PR looks good to me to merge when you're ready, @P-Gill97 .

@P-Gill97
Copy link
Contributor Author

P-Gill97 commented Dec 20, 2023

I got a final UX review on the accordion styles. Maddy pointed out some CSS things she wanted changed.

  • Padding at the bottom of the accordion and content
  • Positioning of the icon
  • Border color change
  • Update styles documentation to recommend that headers should be short.

I fixed these things and will merge into main tomorrow.

@P-Gill97 P-Gill97 merged commit 49b339d into main Dec 21, 2023
1 of 3 checks passed
@P-Gill97 P-Gill97 deleted the k12-552/implement-accordion branch December 21, 2023 18:04
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