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

Upward opening accordions #1578

Merged
merged 12 commits into from
Dec 12, 2023
Merged

Upward opening accordions #1578

merged 12 commits into from
Dec 12, 2023

Conversation

mandla-noredink
Copy link
Contributor

@mandla-noredink mandla-noredink commented Dec 6, 2023

🔧 Modifying a component

Context

This PR makes it so accordions can be set to open either downwards or upwards, to facilitate the notification design in the Premium Vouchers Upsell Notification project.

🖼️ What does this change look like?

Screenshot 2023-12-06 at 10 19 02

Component completion checklist

  • I've gone through the relevant sections of the Development Accessibility guide with the changes I made to this component in mind
  • Changes are clearly documented
    • Component docs include a changelog
    • Any new exposed functions or properties have docs
  • Changes extend to the Component Catalog
    • The Component Catalog is updated to use the newest version, if appropriate
    • The Component Catalog example version number is updated, if appropriate
    • Any new customizations are available from the Component Catalog
    • The component example still has:
      • an accurate preview
      • valid sample code
      • correct keyboard behavior
      • correct and comprehensive guidance around how to use the component
  • Changes to the component are tested/the new version of the component is tested
  • Component API follows standard patterns in noredink-ui
    • e.g., as a dev, I can conveniently add an nriDescription
    • and adding a new feature to the component will not require major API changes to the component
  • If this is a new major version of the component, our team has stories created to upgrade all instances of the old component. Here are links to the stories:
  • Please assign the following reviewers (applicable Sep 2023–Dec 2023):
    • a11y-volunteer-reviewers - Someone from this group will review your PR in full.
    • team-accessibilibats-a11ybats - Someone from this group will review your PR for ACCESSIBILITY ONLY.
    • Someone from your team who can review requirements from your team's perspective. (This could be the same person from the a11y-volunteer-reviewers group if you'd like.)
    • If there are user-facing changes, a designer. (You may want to direct your designer to the deploy preview for easy review):
      • For writing-related component changes, add Stacey (staceyadams)
      • For quiz engine-related components, add Ravi (ravi-morbia)
      • For a11y-related changes to general components, add Ben (bendansby)
      • For general component-related changes or if you’re not sure about something, add the Design group (NoRedInk/design)

@mandla-noredink mandla-noredink self-assigned this Dec 6, 2023
@mandla-noredink mandla-noredink requested review from macoca, ap-nri, ravi-morbia, a team and charbelrami and removed request for a team December 7, 2023 11:59
Copy link

@ravi-morbia ravi-morbia left a comment

Choose a reason for hiding this comment

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

Hey @mandla-noredink! I am not really sure how to review this one. I see the upwards accordion working, but it's pretty far from what we are trying to do in the Dashboard notification.

I am assuming this is a first step towards that. If it is just a better implementation strategy to add the component variant and use it as a building block for the upsell notification, that makes sense.

Just noting that I don't think we would ever use this upwards accordion component as it is here.

Copy link
Contributor

@charbelrami charbelrami left a comment

Choose a reason for hiding this comment

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

Hey @mandla-noredink ! I don't think this should be an accordion in terms of accessibility. The behavior is different as "show more" expands a content that was already partially visible. So SR announcements should be handled in specific way. We can discuss ways of making these notifications accessible. Let me know if I'm missing something.

Edit: Kristine and I looked closer at the figma, and we think an accordion might be a suitable option. Do you want to sync on this @mandla-noredink @ravi-morbia ?

@ravi-morbia
Copy link

Hey @mandla-noredink ! I don't think this should be an accordion in terms of accessibility. The behavior is different as "show more" expands a content that was already partially visible. So SR announcements should be handled in specific way. We can discuss ways of making these notifications accessible. Let me know if I'm missing something.

Edit: Kristine and I looked closer at the figma, and we think an accordion might be a suitable option. Do you want to sync on this @mandla-noredink @ravi-morbia ?

Hey @charbelrami I don't have a real stake in how this is built. Just wanted to note that this variant of the component might never be used on its own. But I could be wrong. Either way, I'm good with this approach if it makes sense from a dev and a11y standpoint.

@mandla-noredink mandla-noredink marked this pull request as ready for review December 12, 2023 15:19
@charbelrami charbelrami self-requested a review December 12, 2023 16:18
Copy link
Contributor

@charbelrami charbelrami left a comment

Choose a reason for hiding this comment

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

@mandla-noredink I think it will be good to merge after implementing @bcardiff suggestion!

@charbelrami charbelrami self-requested a review December 12, 2023 17:06
@charbelrami
Copy link
Contributor

charbelrami commented Dec 12, 2023

Hey @charbelrami I don't have a real stake in how this is built. Just wanted to note that this variant of the component might never be used on its own. But I could be wrong. Either way, I'm good with this approach if it makes sense from a dev and a11y standpoint.

thanks @ravi-morbia! I think that from an a11y and dev perspectives it makes sense in terms os similar behavior and html attributes, even with visual differences

Copy link
Contributor

@charbelrami charbelrami left a comment

Choose a reason for hiding this comment

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

just updating to "request changes"

@mandla-noredink mandla-noredink dismissed charbelrami’s stale review December 12, 2023 22:25

Hey Charbel, the changes have been made, dismissing the review so we can get this merged

@mandla-noredink mandla-noredink merged commit 36001ad into master Dec 12, 2023
5 checks passed
@mandla-noredink mandla-noredink deleted the growth/upward-accordion branch December 12, 2023 22:25
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.

6 participants