Panel: Convert to TypeScript #47259
Merged
Panel: Convert to TypeScript #47259
Conversation
This file contains 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
Not going to add to the Storybook for now because it shouldn't really be used independently. No usages in the repo either.
|
Size Change: +18 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
mirka
commented
Jan 18, 2023
| * This is used by the `Panel` component under the hood, | ||
| * so it does not typically need to be used. | ||
| */ | ||
| function PanelHeader( { label, children }: PanelHeaderProps ) { |
There was a problem hiding this comment.
Not going to add PanelHeader the Storybook for now because it shouldn't really be used independently. No usages in the repo either. If it weren't for back compat I would remove it from the public exports.
There was a problem hiding this comment.
Should we consider deprecating it to discourage further usage?
There was a problem hiding this comment.
The thing is, I doubt anyone is using it because there is no reason to 😆 So it's not that I want to discourage people from using it per se, it's more that I don't want to clutter our docs with something that is irrelevant to 99.9% of people.
| * `PanelRow` is a generic container for rows within a `PanelBody`. | ||
| * It is a flex container with a top margin for spacing. | ||
| */ | ||
| export const _PanelRow = Template.bind( {} ); |
There was a problem hiding this comment.
I added this story to clarify what the default styling of PanelRow looks like. I don't quite understand why it is space-between, but that's something we'll have to keep for back compat. Also the 8px margin-top seems way too narrow.
Though I'm wondering whether we can somehow salvage and update this component to use as #43423 🤔 Something to think about!
There was a problem hiding this comment.
Good point! I think that Panel overall needs a refresh (or a deprecation), especially since more and more of our UI uses ToolsPanel. Something that we could at some point discuss with @jasmussen
|
Flaky tests detected in 3346967. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4077213550
|
chad1008
reviewed
Jan 24, 2023
You're right, most of the test files can already be converted! Done in edab3c1 👍 The story file and the test file for PanelBody will have to wait until PanelBody is typed. (Btw an intentional but quirky thing about the wp-components tsconfig setup is that all files are typechecked by default regardless of file extension, except for tests/stories which need to be |
ciampo
reviewed
Jan 26, 2023
| * This is used by the `Panel` component under the hood, | ||
| * so it does not typically need to be used. | ||
| */ | ||
| function PanelHeader( { label, children }: PanelHeaderProps ) { |
There was a problem hiding this comment.
Should we consider deprecating it to discourage further usage?
|
|
||
| /** | ||
| * `PanelRow` is a generic container for rows within a `PanelBody`. | ||
| * It is a flex container with a top margin for spacing. |
There was a problem hiding this comment.
I understand that these info can be useful for a dev using this component, but should we expose implementation details like layout APIs (flex) and design details (margin top) ? An interesting dilemma, I personally don't have a strong opinion
There was a problem hiding this comment.
For this one I decided it was necessary to mention because it's essential to what PanelRow is. Those two styles are the only reason the component exists, and I phrased it in a way that we can stick to as a contract. In other words, if it ceases to be a flex container, or if it ceases to have a top margin, I would consider it a breaking change.
| * `PanelRow` is a generic container for rows within a `PanelBody`. | ||
| * It is a flex container with a top margin for spacing. | ||
| */ | ||
| export const _PanelRow = Template.bind( {} ); |
There was a problem hiding this comment.
Good point! I think that Panel overall needs a refresh (or a deprecation), especially since more and more of our UI uses ToolsPanel. Something that we could at some point discuss with @jasmussen
It's probably too soon to deprecate it. It's still useful in the main post inspector (Categories, tags, featured image, etc.) and I believe it's also used in the prepublish flow still. It seems fine to refactor and optimize, we can't be sure that all of it will be replaced with tools panels. There might still be use cases for the collapsible. |
ciampo
approved these changes
Feb 1, 2023
|
Looks like playwright CI tasks got stuck.. maybe you can push a new commit to force them to re-run ? |
Huh, that's strange. |
DaisyOlsen
added
the
[Type] Code Quality
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.
Part of #35744
What?
Converts Panel, PanelHeader, and PanelRow to TypeScript.
💁♀️ PanelBody will be done in a separate PR for easier review.
How?
This one is really straightforward in terms of types. No runtime changes.
Testing Instructions
npm run storybook:dev