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

Honor icon/rightIcon on entries with children #1613

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

bcardiff
Copy link
Member

@bcardiff bcardiff commented Jan 30, 2024

🔧 Modifying a component

Context

As part of KRA-1345 we are going to use SideNav.icon for the emoji. We noticed that the icons are not rendered for entries with children. This PR fixes that.

🖼️ What does this change look like?

Before this PR (with both entries using SideNav.icon)
Screenshot 2024-01-30 at 17 58 48

After this PR

Screenshot 2024-01-30 at 17 58 30

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:
    • add your story links here OR just write this is not a new major version
  • Please assign the following reviewers:
    • Someone from your team who can review your PR in full and review requirements from your team's perspective.
    • team-accessibilibats-a11ybats - Someone from this group will review your PR for accessibility and adherence to component library foundations.
    • 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)

Copy link

linear bot commented Jan 30, 2024

@bendansby
Copy link
Contributor

What about the children themselves?

@bcardiff
Copy link
Member Author

I haven't test/change them, but I believe if they have an icon defined they will show up.

For interest page only top entries have icons though.

I will check for completeness early tomorrow

@bendansby
Copy link
Contributor

Regarding https://github.com/NoRedInk/NoRedInk/pull/47192 can the example in the component catalog be updated to reflect that an icon can either be a UiIcon or, what, arbitrary html, unicode number?

@bcardiff
Copy link
Member Author

Children with icons will show up as expected. If only some have them then they will look missaligned.

Screenshot 2024-01-31 at 10 30 50

Regarding https://github.com/NoRedInk/NoRedInk/pull/47192 the workaround I am doing is create an svg on the fly with <text> element that contains only the emoji. See https://github.com/NoRedInk/NoRedInk/pull/47192/files#diff-868dcf984c8a1ff852da0f392c7047ba4a764c347430d0b6b3262fac52c349ccR2155-R2165 . I don't think it's something we should make oficial in noredink-ui. Don't we want real svg icons eventually to have consistent look and feel instead of relying on browsers/os emoji that vary?

@bendansby
Copy link
Contributor

Yes, that makes sense. I just wasn’t sure what you had implemented there. But no action needed then.

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.

thanks!

@bcardiff bcardiff merged commit 8be88cc into master Jan 31, 2024
8 checks passed
@bcardiff bcardiff deleted the add-icons-to-sidenav-parents branch January 31, 2024 16:47
@bcardiff bcardiff mentioned this pull request Jan 31, 2024
1 task
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.

None yet

3 participants