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-1637: Update Content Display Chakra #1471

Merged
merged 5 commits into from
Dec 21, 2023

Conversation

isastettler
Copy link
Collaborator

Fixes JIRA ticket xxx

This PR does the following:

  • Update Component type for HelperErrorText and StatusBadge
  • Update component style file for helperErrorText and statusBadge

How has this been tested?

Locally

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.

Copy link

vercel bot commented Dec 6, 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 Dec 6, 2023 5:27pm

@isastettler isastettler added the Needs Review Pull requests that are ready for peer review. label Dec 6, 2023
Copy link

github-actions bot commented Dec 6, 2023

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

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.

The update itself looks great, thanks!

The only thing missing is changelog updates. Can you update the respective storybook files to include "prerelease" in the version table and also add helperErrorTextChangelogData.ts and statusBadgeChangelogData.ts files? They can similar to this: https://github.com/NYPL/nypl-design-system/pull/1468/files#diff-8bc92befba3589679a49736327283e9bf375f12582239b94874b15c9312ce479
and then added to a changelog section in the .mdx file at the very end.

import { createMultiStyleConfigHelpers } from "@chakra-ui/styled-system";
import { defineStyle, StyleFunctionProps } from "@chakra-ui/system";

interface HelperErrorTextBaseStyle extends StyleFunctionProps {
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense and I see why I didn't need this in the Checkbox PR, this is needed when you are passing props to the theme functions.

import { createMultiStyleConfigHelpers } from "@chakra-ui/styled-system";
import { defineStyle, StyleFunctionProps } from "@chakra-ui/system";

interface HelperErrorTextBaseStyle extends StyleFunctionProps {
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI, I ran into an issue where this caused a problem:

Screen Shot 2023-12-06 at 4 55 17 PM

It's not a problem for this component or interface but if you run into the same problem, it can be fixed by making it a partial Partial<StyleFunctionProps>. We do need to extend the interface but we don't need to make all the properties required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To keep it consistent I make the change here too. It might be better in the long run.

Copy link
Member

Choose a reason for hiding this comment

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

We talked about this offline but for history: we couldn't figure out why Chakra is being inconsistent and why not using Partial<StyleFunctionProps> in this case breaks. So for this case (it might be a case-by-case basis), it's okay to not use Partial.

@bigfishdesign13 bigfishdesign13 added Ship It Pull requests that have been reviewed and approved. DON’T MERGE YET When somethign has been approved, but should not be merged just yet. and removed Needs Review Pull requests that are ready for peer review. DON’T MERGE YET When somethign has been approved, but should not be merged just yet. labels Dec 13, 2023
@isastettler isastettler merged commit 89a9b8a into react18-and-chakra Dec 21, 2023
5 checks passed
@EdwinGuzman EdwinGuzman deleted the DSD-1637/content-display-ckara-update branch January 31, 2024 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It Pull requests that have been reviewed and approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants