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(combobox/text-input): add svg top positioning for IE11 #5362

Merged
merged 13 commits into from
Feb 18, 2020
Merged

fix(combobox/text-input): add svg top positioning for IE11 #5362

merged 13 commits into from
Feb 18, 2020

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented Feb 15, 2020

partially addresses #5335

Adds top attribute to SVG positioning in text-input and combobox getting that SVG sitting properly in the form element. Also adds the css-reset to combobox so that when #5357 lands the text-input will display: none for ::-ms-clear getting rid of that big funky browser default X 👍

Testing / Reviewing

check the clear button positioning in IE11 for both text-input and combobox. Then check the clear button positioning in modern browsers just to be safe 🦅

Note

I didn't stick the top attribute into a query for IE11 specifically because adding the top attribute had no negative effect on Firefox and Chrome. If that wont fly let me know 💟

@dakahn dakahn requested a review from a team February 15, 2020 00:07
@dakahn dakahn requested a review from a team as a code owner February 15, 2020 00:07
@dakahn dakahn requested review from asudoh and joshblack and removed request for a team February 15, 2020 00:07
@netlify
Copy link

netlify bot commented Feb 15, 2020

Deploy preview for carbon-elements ready!

Built with commit 4d68562

https://deploy-preview-5362--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Feb 15, 2020

Deploy preview for carbon-components-react ready!

Built with commit 4d68562

https://deploy-preview-5362--carbon-components-react.netlify.com

@asudoh asudoh requested review from a team and laurenmrice and removed request for a team February 15, 2020 01:06
@laurenmrice
Copy link
Member

I checked all the other browsers just in case and they look good. Will see if I can check ie11 with josh today (since I can't view on my comp)

@@ -343,6 +343,7 @@ $list-box-menu-width: rem(300px);
.#{$prefix}--list-box__selection {
position: absolute;
right: rem(33px); // to preserve .5rem space between icons according to spec
top: 0.35rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use bespoke positioning or could we do top: 0; bottom: 0; to center?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me check 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we settled on top: 50%; bottom: translateY(-50%); 👍

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Seems great once text input is updated!

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @dakahn!

@tw15egan tw15egan merged commit 347185b into carbon-design-system:master Feb 18, 2020
@emyarod
Copy link
Member

emyarod commented Feb 20, 2020

this causes a regression for multiselects

image

@asudoh
Copy link
Contributor

asudoh commented Feb 20, 2020

Oh looks a good catch - Have you reported it @emyarod?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants