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 Pennant V4 #1674

Merged
merged 4 commits into from
May 15, 2024
Merged

Add Pennant V4 #1674

merged 4 commits into from
May 15, 2024

Conversation

bendansby
Copy link
Contributor

@bendansby bendansby commented May 10, 2024

🔧 Modifying a component

Context

Makes the pennant icons flush with their bounding boxes so they don't have to be eyeballed. I'm not deprecating V3 since they shouldn't be upgraded without a designer's eye.

🖼️ What does this change look like?

image

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.
    • Component library owner - 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)

@bendansby
Copy link
Contributor Author

@asterite I added you since this would be good to upgrade when we add the wand

elm.json Outdated
"Nri.Ui.Pennant.V3",
"Nri.Ui.Pennant.V4",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think CI is failing because both these lines need to be here (so both are exposed and we can use them)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why, right. Fixed!

@asterite
Copy link
Contributor

Next error is:

Command line: elm-forbid-import check
Exit code: 1
Stderr and Stdout:
src/Nri/Ui/PremiumCheckbox/V8.elm:68:7:forbidden import Nri.Ui.Pennant.V3 (upgrade to V4)
src/Nri/Ui/RadioButton/V4.elm:74:7:forbidden import Nri.Ui.Pennant.V3 (upgrade to V4)
src/Nri/Ui/SideNav/V5.elm:84:7:forbidden import Nri.Ui.Pennant.V3 (upgrade to V4)
component-catalog/src/Examples/PremiumCheckbox.elm:25:7:forbidden import Nri.Ui.Pennant.V3 (upgrade to V4)

If these are too much to handle right now (or you intended to import a forbidden
module), please run me with the `update` command!

I guess we don't want to upgrade to V3 right away in these components? In that case maybe running elm-forbid-import update then committing that change would make it work (also let me know if you'd like me to do that)

@bendansby bendansby removed the request for review from mandla-noredink May 15, 2024 12:53
@bendansby
Copy link
Contributor Author

If you could, that'd be appreciated!

@asterite
Copy link
Contributor

Hm, I tried doing that but it gives no error locally 🤷

I think someone more familiar with noredink-ui should take a look at this.

@bendansby
Copy link
Contributor Author

I don't know that anyone is an expert on noredink-ui, but I'll ask in Slack.

@bendansby
Copy link
Contributor Author

Thanks @bcardiff! cc @asterite

Copy link
Contributor

@asterite asterite left a comment

Choose a reason for hiding this comment

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

@bendansby bendansby merged commit 10d83bb into master May 15, 2024
8 checks passed
@bendansby bendansby deleted the dansby/pennant-v4 branch May 15, 2024 18:28
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