Skip to content

Conversation

@lsl
Copy link
Contributor

@lsl lsl commented Jan 17, 2022

Description

Reduces the height of the reset button icon so that it doesn't expand the textbox height that contains it.

This expanded height was noticed when we placed it next to a text input we expected would have a matching height (example, it's off by 2px).

How has this been tested?

Manually checking in storybook docs

npm run storybook:dev

Screenshots

Before

allowReset-before.mp4

After

allowReset-after.mp4

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@lsl lsl requested a review from ajitbohra as a code owner January 17, 2022 07:00
@cbravobernal cbravobernal added [Package] Components /packages/components Storybook Storybook and its stories for components labels Jan 17, 2022
@cbravobernal cbravobernal requested a review from kjellr January 17, 2022 14:34
@ciampo ciampo self-requested a review January 24, 2022 09:32
@ciampo ciampo requested a review from mirka January 24, 2022 09:49
@ciampo ciampo changed the title Fix allowReset x height ComboBox: fix reset button's height Jan 24, 2022
Copy link
Contributor

@ciampo ciampo 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 @lsl for working on this!

I left an inline comment, but in general the changes test well as per the instructions.

Would you mind also adding an entry to the CHANGELOG ?

Comment on lines 265 to 267
Copy link
Contributor

@ciampo ciampo Jan 24, 2022

Choose a reason for hiding this comment

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

The knobs addon is deprecated and we're in the process of converting the examples to the new controls approach.

Would you mind refactoring this Storybook example? Here's a couple of references:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the links, will take a look later this week.

@lsl
Copy link
Contributor Author

lsl commented Jan 31, 2022

@ciampo Updated to remove knobs usage + added changelog.

@ciampo ciampo added the [Type] Bug An existing feature does not function as intended label Jan 31, 2022
Copy link
Contributor

@ciampo ciampo 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 @lsl for working on this! Code changes LGTM, and the Storybook example is configured correctly with the Controls.

I believe this PR needs to be rebased before merging — while doing so, could you also move the CHANGELOG entry under a category (e.g. ### Bug fix)? Thank you!

@lsl
Copy link
Contributor Author

lsl commented Feb 1, 2022

@ciampo, done! Thanks for the help!

@ciampo ciampo merged commit 8fcd7f2 into WordPress:trunk Feb 1, 2022
@github-actions github-actions bot added this to the Gutenberg 12.6 milestone Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package] Components /packages/components Storybook Storybook and its stories for components [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants