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

UnitControl : Deprecate 36px default size #66791

Merged

Conversation

hbhalodia
Copy link
Contributor

Part of #65751

What?

Deprecate the 36px default size in UnitControl.

Testing Instructions

  • Unit tests pass
  • Storybook stories should not log console warnings
  • All code snippets in documentation (JSDoc, Storybook, README) should have the __next40pxDefaultSize prop enabled

Copy link

github-actions bot commented Nov 6, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: hbhalodia <[email protected]>
Co-authored-by: mirka <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@hbhalodia
Copy link
Contributor Author

Hi @mirka, Some of this unit test are failing because, this UnitControl component is used inside BoxControl component, So once we add this PR - #66704, then I guess then it would have __next40pxdefaultsize prop being passed with component as prop drill and unit test would not fail.

Let me know if we have some other approach to handle this case?

Thank You,

@hbhalodia hbhalodia requested a review from mirka November 28, 2024 05:59
@hbhalodia
Copy link
Contributor Author

Hi @mirka, Some of this unit test are failing because, this UnitControl component is used inside BoxControl component, So once we add this PR - #66704, then I guess then it would have __next40pxdefaultsize prop being passed with component as prop drill and unit test would not fail.

Let me know if we have some other approach to handle this case?

Thank You,

This is now fixed, checked the comment - #65751 (comment), and updated the PR to not log warning from the child component used.

Thank You,

@mirka mirka added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components Needs Dev Note Requires a developer note for a major WordPress release cycle labels Nov 28, 2024
@hbhalodia hbhalodia requested a review from mirka November 29, 2024 05:28
@hbhalodia
Copy link
Contributor Author

Hi @mirka, Updated the PR with requested changes and the suggestions 🙇.

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Thank you for the meticulous work! I'm just going to revert the changes on font-size-picker/index.native.js, because *.native.js files are for the mobile app and are mostly unrelated to the normal component code we're working on.

@mirka mirka enabled auto-merge (squash) November 29, 2024 14:46
@hbhalodia
Copy link
Contributor Author

Thank you for the meticulous work! I'm just going to revert the changes on font-size-picker/index.native.js, because *.native.js files are for the mobile app and are mostly unrelated to the normal component code we're working on.

Got the point 👍

@mirka mirka merged commit edd6328 into WordPress:trunk Nov 29, 2024
63 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.9 milestone Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Dev Note Requires a developer note for a major WordPress release cycle [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