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

Docs: Fix Typeset Doc block fontSizes type #26475

Merged
merged 2 commits into from
May 15, 2024

Conversation

noranda
Copy link
Contributor

@noranda noranda commented Mar 13, 2024

What I did

Fixes the type for fontSizes in the Typeset block to match the docs.

Currently, Typescript will throw an error if you try to pass in a number[] to this prop. You either need to typecast a number[] to a string[], or pass in a string array with the px in order for it to work.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  1. Run any app or sandbox that uses Storybook, , e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts.
  2. Open any editor that has Typescript support.
  3. Create a component that uses the Typeset component and pass in an array of numbers to fontSizes.
  4. Should not see any Typescript errors.

Documentation

No updates to documentation needed.

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

@jonniebigodes
Copy link
Contributor

@noranda appreciate you taking the time to put together this pull request. We appreciate it 🙏 ! @kylegach when you have a moment can you check this and follow up with @JReinhold and the contributor for documentation. Thanks in advance.

@@ -35,7 +35,7 @@ const Wrapper = styled.div(withReset, ({ theme }) => ({

export interface TypesetProps {
fontFamily?: string;
fontSizes: string[];
fontSizes: number[];
Copy link
Contributor

Choose a reason for hiding this comment

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

The proper type would actually be (string | number)[] because that's the type of the underlying data model. It would be great if you could update the docs to reflect this as well.

Suggested change
fontSizes: number[];
fontSizes: (string | number)[];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JReinhold Thanks! All set!

@noranda noranda force-pushed the nb/fix-typeset-fontsizes-type branch from a7413a1 to e1a3ba6 Compare May 8, 2024 16:55
@noranda noranda force-pushed the nb/fix-typeset-fontsizes-type branch from e1a3ba6 to 46e3617 Compare May 8, 2024 16:56
Copy link
Contributor

@JReinhold JReinhold 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. We've just entered feature freeze for 8.1, so we wont be merging this until next week.

Copy link

nx-cloud bot commented May 8, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4496b48. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@JReinhold JReinhold changed the title Fix: Typeset fontSizes type to match docs Docs: Fix Typeset Doc block fontSizes type May 15, 2024
@JReinhold JReinhold merged commit 3e5fb46 into storybookjs:next May 15, 2024
50 of 52 checks passed
@github-actions github-actions bot mentioned this pull request May 15, 2024
13 tasks
@noranda noranda deleted the nb/fix-typeset-fontsizes-type branch May 15, 2024 13:54
@valentinpalkovic valentinpalkovic added the needs qa Indicates that this needs manual QA during the upcoming minor/major release label Jul 5, 2024
@ghengeveld ghengeveld removed the needs qa Indicates that this needs manual QA during the upcoming minor/major release label Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants