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

DSD-1646: Updating NewsletterSignUp, Radio, RadioGroup, and Searchbar to Chakra 2.8 theme API #1480

Merged
merged 9 commits into from
Jan 8, 2024

Conversation

7emansell
Copy link
Collaborator

@7emansell 7emansell commented Dec 19, 2023

Fixes JIRA ticket DSD-1646

This PR does the following:

  • Updates NewsletterSignUp, Radio, RadioGroup, and Searchbar components to use the Chakra 2.8 updated API for their theme object.
  • No functional changes, only theme file

How has this been tested?

Locally and compared against the production Storybook instance.

Accessibility concerns or updates

N/A

Checklist:

  • I have updated the Storybook documentation accordingly.
  • I have added relevant accessibility documentation for this pull request.
  • All new and existing tests passed.

Front End Review:

  • Review the Vercel preview deployment once it is ready.

@7emansell 7emansell added the In Progress Tickets or PRs that are being worked on. PRs marked as In Progress should not be merged. label Dec 19, 2023
Copy link

vercel bot commented Dec 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nypl-design-system ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 8, 2024 3:55pm

Copy link

Your pull request is missing a changelog—was that intentional?

@7emansell 7emansell changed the title DSD-1646 Updating NewsletterSignUp, Radio, RadioGroup, and Searchbar to Chakra 2.8 theme API DSD-1646: Updating NewsletterSignUp, Radio, RadioGroup, and Searchbar to Chakra 2.8 theme API Dec 19, 2023
@7emansell 7emansell removed the In Progress Tickets or PRs that are being worked on. PRs marked as In Progress should not be merged. label Dec 20, 2023
@7emansell
Copy link
Collaborator Author

Noting that the Searchbar doesn't fill the given width as it should (and does on prod) but that's how it appears on my local instance regardless of Chakra update? Also noting that the NewsletterSignUp looks off (button, heading) but those updates are coming on those components' respective tickets.

Copy link
Member

@EdwinGuzman EdwinGuzman left a comment

Choose a reason for hiding this comment

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

Looks good to me pending updates to the Heading and Button components which will be updated in other PRs. I think that's also what's happening in SearchBar where the TextInput component needs an update but that's a separate PR, so it makes sense that it's not full width just yet.

package.json Outdated
"@chakra-ui/react": "2.8.1",
"@chakra-ui/system": "2.6.2",
"@chakra-ui/focus-lock": ">=2.1.0",
"@chakra-ui/react": ">=2.8.1",
Copy link
Member

Choose a reason for hiding this comment

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

Did you make all the ">=" updates? Did you encounter any issues while installing packages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops yes accidentally, I had something up with esbuild, they're locked now

package.json Outdated
@@ -60,6 +60,7 @@
"@emotion/react": "11.11.1",
"@emotion/styled": "11.11.0",
"downshift": "6.1.7",
"esbuild": "^0.19.10",
Copy link
Member

Choose a reason for hiding this comment

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

I had some esbuild issues as well. Can you try running this without esbuild? If we can remove this dependency, that'd be great, otherwise it's okay to keep.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's running fine without esbuild now so I removed it.. weird though

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