Merged
Conversation
walbo
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for working on this one @walbo 👍
Everything tests as advertised for me. I left a couple of minor suggestions but am happy to approve if we decide the prop descriptions are ok as they are.
✅ No typing errors
✅ Unit tests pass
✅ Code changes make sense
✅ Has changelog
✅ Component functions correctly in both Storybook and editors
✅ Storybook example has appropriate controls
❔ README matches type props and descriptions
❔ Auto-generated docs in Storybook are meaningful
mirka
reviewed
Jun 24, 2022
Member
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
Looks good in general!
TypeScript is telling me that gridColumnGap/gridRowGap are deprecated 😮 Apparently they were renamed in CSS3? Let's just fix that up for spec compliance.
/**
- * Adjusts the `grid-column-gap`.
+ * Adjusts the `column-gap`.
*/
- columnGap?: CSSProperties[ 'gridColumnGap' ];
+ columnGap?: CSSProperties[ 'columnGap' ];
Contributor
I have a question about this. The This is assuming we're trying to strictly follow the guidelines in not making runtime changes during TypeScript refactors. It's only a very minor change of course, hence this just being a question 🙂 |
Member
Ah you're right, I was just thinking about types but it makes sense to change the CSS code as well. Let's move it to a follow-up then 👍 |
aaronrobertshaw
approved these changes
Jun 28, 2022
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for the refinements to this @walbo 👍
This is looking pretty good.
✅ No typing errors
✅ Unit tests pass
✅ Component functions well in editor and storybook
I'm happy to give this a tentative approval. My only remaining suggestion is in relation to the displayed type in the component's storybook docs.
In the meantime, could we maybe just pretend, for the casual docs reader, that it only takes a number?
This PR currently does pretend in the README that columns and rows only take number values. In the auto-generated storybook docs however it still shows as ResponsiveCSSValue< number >.
Would it avoid some confusion and further the efforts to obfuscate the responsive values type if we updated the storybook controls table to only list number as the type as well?
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
ciampo
reviewed
Jun 28, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Not much to add to the very comprehensive reviews from @aaronrobertshaw and @mirka !
Let's move it to a follow-up then 👍
Agreed, let's follow-up with those changes to the gridRowGap and gridColumnGap props
Member
Author
|
Thanks for the feedback and reviews 👍 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
What?
Converts the
Gridcomponent to TypeScript.Why?
Part of the @wordpress/components's TypeScript migration (#35744).
How?
Gridto TypeScript.Testing Instructions
Gridcontinues to function as expected