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

Heading: refactor away from the createComponent function, fix TS errors #34921

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Sep 17, 2021

Description

Fixes partially #34920.

How has this been tested?

Technically there should be no behavioral changes.

The project should build, the tests should pass, the component should behave as before in Storybook and in the block editor.

Types of changes

Refactor

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • N/A I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@ciampo ciampo changed the title Heading: refactor away from the createComponent function, fix TS … Heading: refactor away from the createComponent function, fix TS errors Sep 17, 2021
@@ -25,7 +25,7 @@ export interface HeadingProps extends Omit< TextProps, 'size' > {
/**
* `Heading` will typically render the sizes `1`, `2`, `3`, `4`, `5`, or `6`, which map to `h1`-`h6`.
*
* @default 3
* @default 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the useHeading hook (defined in this same file), the default value of level is 2

@@ -56,16 +56,17 @@ export function useHeading(
'Heading'
);

const as = asProp || `h${ level }`;
const as = ( asProp || `h${ level }` ) as keyof JSX.IntrinsicElements;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Necessary to avoid a TypeScript error when passing this computed as prop to the View component

Comment on lines +63 to +69
'aria-level'?: number;
} = {};
if ( typeof as === 'string' && as[ 0 ] !== 'h' ) {
// if not a semantic `h` element, add a11y props:
a11yProps.role = 'heading';
a11yProps[ 'aria-level' ] = level;
a11yProps[ 'aria-level' ] =
typeof level === 'string' ? parseInt( level ) : level;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix a TypeScript error where the expected value of aria-level can only be number | undefined (and not string)

@ciampo ciampo self-assigned this Sep 17, 2021
@ciampo ciampo added [Feature] Component System WordPress component system [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality labels Sep 17, 2021
@ciampo ciampo marked this pull request as ready for review September 17, 2021 17:35
@ciampo ciampo merged commit 4474d01 into trunk Sep 20, 2021
@ciampo ciampo deleted the feat/components-refactor-create-component-heading branch September 20, 2021 14:44
@github-actions github-actions bot added this to the Gutenberg 11.6 milestone Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants