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

fix(textbox): fixed large textbox height #2392

Merged
merged 5 commits into from
Jul 25, 2024
Merged

fix(textbox): fixed large textbox height #2392

merged 5 commits into from
Jul 25, 2024

Conversation

ArtBlue
Copy link
Contributor

@ArtBlue ArtBlue commented Jul 24, 2024

Fixes #2389

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

Fixes large textbox height.

Screenshots

Before

Screenshot 2024-07-24 at 8 44 13 AM

After

Screenshot 2024-07-24 at 8 45 01 AM

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I did a visual regression check of the components impacted by doing a Percy build and approved the build
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

@ArtBlue ArtBlue requested review from agliga and LuLaValva July 24, 2024 15:54
@ArtBlue ArtBlue self-assigned this Jul 24, 2024
Copy link

changeset-bot bot commented Jul 24, 2024

🦋 Changeset detected

Latest commit: 058d05f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@ebay/skin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ianmcburnie
ianmcburnie previously approved these changes Jul 24, 2024
Copy link
Contributor

@ianmcburnie ianmcburnie left a comment

Choose a reason for hiding this comment

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

LGTM, just double check across all tier 1 browsers if not done so already.

Comment on lines 200 to 202
.textbox--large > input.textbox__control {
height: 48px;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is also a problem for .textbox (without --large). I like moving the size up to the parent container, but if you are doing that you should also change the child.

These are the changes I think we need:

  • .textbox { height: 40px }
  • .textbox__control { height: 100% }
  • Remove this selector and its contents (.textbox--large > input.textbox__control)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LuLaValva , I reworked this based on our discussion.

@ArtBlue ArtBlue requested a review from LuLaValva July 24, 2024 23:33
Copy link
Member

@LuLaValva LuLaValva left a comment

Choose a reason for hiding this comment

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

Thanks for the back-and-forth, looks good now!

@ArtBlue ArtBlue merged commit 781ee73 into master Jul 25, 2024
@ArtBlue ArtBlue deleted the bug-textbox-height branch July 25, 2024 00:03
@github-actions github-actions bot mentioned this pull request Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

textbox: Different size between large select and input text
3 participants