UnitControl: mark unit prop as deprecated
#39503
Merged
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
[Type] Developer Documentation
|
Size Change: +75 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
This was referenced Mar 16, 2022
stokesman
approved these changes
Mar 16, 2022
Contributor
stokesman
left a comment
stokesman
left a comment
There was a problem hiding this comment.
Looks and tests fine for me.
8 tasks
ciampo
force-pushed
the
feat/unit-control-deprecate-unit
branch
from
d055c30 to
d75a6be
Compare
andrewserong
approved these changes
Mar 21, 2022
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
Thanks for the follow-up with all the one-off PRs! This is testing nicely for me @ciampo, the UnitControl component behaves as it does on trunk in Storybook and within the instances in the editor that I tested. If I manually add a unit prop to e.g. the height control in the Spacer block, then when the component is mounted I get the following warning in the console 👍
A non-blocking question: I was wondering if it's worth retaining at least one unit test that tests that the deprecated unit prop still works, but then also check that the warning is generated (e.g. via expect( console ).toHaveWarnedWith()). Probably not a huge issue, but this would be a test to ensure that we don't accidentally remove the feature during its status as "still working but deprecated". I'm not too sure how frequently we have those sorts of tests in the Gutenberg repo? (I couldn't find many examples outside of the deprecated package), but just thought I'd raise the question 😃
aaronrobertshaw
approved these changes
Mar 21, 2022
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Nice work tying up all the loose ends with the UnitControl @ciampo 👍
This is testing well for me.
✅ Code changes LGTM
✅ Readme and types match in terms of deprecating unit prop
✅ Unit tests pass
✅ Deprecation warning thrown when adding unit prop (tested via cover block min-height and storybook examples)
✅ Couldn't see any functional differences between this PR and trunk
I was wondering if it's worth retaining at least one unit test that tests that the deprecated unit prop still works, but then also check that the warning is generated
This sounds like a good idea for a follow-up to me.
Co-authored-by: Mitchell Austin <[email protected]>
ciampo
force-pushed
the
feat/unit-control-deprecate-unit
branch
from
d75a6be to
ebf89dd
Compare
Contributor
Author
Good point, will do 👌 |
jostnes
pushed a commit
to jostnes/gutenberg
that referenced
this pull request
Mar 23, 2022
* `UnitControl`: mark `unit` prop as deprecated * Update 's unit tests * CHANGELOG * Update comment Co-authored-by: Mitchell Austin <[email protected]> * Use `in` operator to check if the deprecated `unit` prop is passed to `UnitControl` Co-authored-by: Mitchell Austin <[email protected]>
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.
What?
In the context of #35744, this PR marks the
unitprop inUnitControlas deprecated.Note: these changes only apply to the web version of
UnitControl(the native version is still relying on theunitprop at the time of working on this PR).Note: this PR requires that all usages of the
UnitControlcomponent (web version) are refactored to avoid using theunitprop:FocalPointPicker: stop usingUnitControl's deprecatedunitprop #39504BoxControl: stop usingUnitControl's deprecatedunitprop #39511Spacerblock: stop usingUnitControl's deprecatedunitprop #39513Searchblock: stop usingUnitControl's deprecatedunitprop #39514Coverblock: stop usingUnitControl's deprecatedunitprop #39522BorderRadiusControl: Stop usingUnitControl’s deprecatedunitprop #39549Why?
The unit prop was marked as deprecated in the README, but this was not reflected in the docs
How?
unitprop as@deprecatedin the TypeScript typesTesting Instructions
No runtime/behavioral changes: