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

ComboBox: fix reset button's height #38020

Merged
merged 3 commits into from
Feb 1, 2022

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
parameters: {
knobs: { disable: false },
},
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 lsl force-pushed the update/combo-box-reset-button branch 2 times, most recently from 87a7151 to 0c94627 Compare January 31, 2022 06:40
@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 lsl force-pushed the update/combo-box-reset-button branch from 0c94627 to 72fa268 Compare February 1, 2022 03:02
@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