Components: refactor away and delete the createComponent function
#34331
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciampo
added
[Feature] Component System
ciampo
commented
Aug 26, 2021
|
Size Change: -29 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
Contributor
sarayourfriend
left a comment
sarayourfriend
left a comment
There was a problem hiding this comment.
Looks good to me! Just one note about the displayName but doesn't really matter too much.
Contributor
Author
Awesome, thank you for taking a look! I will proceed with the rest of the components |
ciampo
force-pushed
the
feat/components-remove-create-component-function
branch
from
17f00ce to
ff83057
Compare
ciampo
commented
Aug 26, 2021
| * `Heading` will typically render the sizes `1`, `2`, `3`, `4`, `5`, or `6`, which map to `h1`-`h6`. | ||
| * | ||
| * @default 3 | ||
| * @default 2 |
Contributor
Author
There was a problem hiding this comment.
In the useHeading hook (defined in this same file), the default value of level is 2
ciampo
commented
Aug 26, 2021
| ); | ||
|
|
||
| const as = asProp || `h${ level }`; | ||
| const as = ( asProp || `h${ level }` ) as keyof JSX.IntrinsicElements; |
Contributor
Author
There was a problem hiding this comment.
Necessary to avoid a TypeScript error when passing this computed as prop to the View component
ciampo
commented
Aug 26, 2021
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; |
Contributor
Author
There was a problem hiding this comment.
Fixes a TypeScript error where the expected value of aria-level can only be number | undefined (and not string)
ciampo
commented
Aug 27, 2021
| */ | ||
| export default function useTruncate( props ) { | ||
| const { | ||
| as = 'span', |
Contributor
Author
There was a problem hiding this comment.
Previously, the Truncate component was being rendered as a span when calling the createComponent function. That function call is being removed in this PR, and so one way to render as a span is to default to it in the hook
ciampo
force-pushed
the
feat/components-remove-create-component-function
branch
from
4acf8e9 to
ace5aa7
Compare
ciampo
commented
Aug 27, 2021
| */ | ||
| export function useControlLabel( props ) { | ||
| const { | ||
| as = 'label', |
Contributor
Author
There was a problem hiding this comment.
Same motivation as #34331 (comment), but for the ControlLabel component, and with a default value for as of label
Contributor
Author
|
This PR is now ready for review. I've left inline comments in all of the parts where the changes were not trivial. Also, deleting the |
ciampo
force-pushed
the
feat/components-remove-create-component-function
branch
2 times, most recently
from
e4d83f6 to
e3eb15c
Compare
Contributor
Author
|
Rebased and solved conflicts after merging #34330 |
ciampo
force-pushed
the
feat/components-remove-create-component-function
branch
from
e3eb15c to
e3c6326
Compare
ciampo
commented
Sep 9, 2021
| * ``` | ||
| */ | ||
| const Text = createComponent( { | ||
| as: 'span', |
Contributor
Author
There was a problem hiding this comment.
This was refactored by assigning span as default value of as in the useText hook
Contributor
There was a problem hiding this comment.
Why the refactor? It's not really equivalent to what was happening before (equivalent would be to add it directly to the View as="span" I think).
Contributor
Author
There was a problem hiding this comment.
I see. Does it mean that, for components using the createComponent function, the as prop is always going to be hardcoded? For example, the Text component will always be rendered as a span, even if we render it as <Text as="p" /> ?
Contributor
Author
|
This PR has been stale for a while. I've just rebased and pushed a couple of tweaks, it'd be great if it could get reviewed soon 🙇♂️ |
Contributor
sarayourfriend
left a comment
sarayourfriend
left a comment
There was a problem hiding this comment.
This is hard to test, it's such a big PR. By any chance would it be possible to split this PR into smaller more easily testable bits? That way you will probably get more traction from different people testing each PR.
| * ``` | ||
| */ | ||
| const Text = createComponent( { | ||
| as: 'span', |
Contributor
There was a problem hiding this comment.
Why the refactor? It's not really equivalent to what was happening before (equivalent would be to add it directly to the View as="span" I think).
Contributor
Author
I will close this PR and open a series of smaller PRs — one for each of the 14 components, plus the final one to delete the |
johnbillion
deleted the
feat/components-remove-create-component-function
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR deletes the
createComponentutility function, used by some components in the@wordpress/componentspackage.How has this been tested?
Screenshots
N/A
Types of changes
Refactor (deleting + refactoring code)
Checklist:
*.native.jsfiles for terms that need renaming or removal).Closes #34290