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

Update input width in _chips.scss sass component #6567

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Git-Harshit
Copy link

@Git-Harshit Git-Harshit commented Jun 5, 2020

This is a minor but suitable change which updates the width of input field (for adding new chips) from fixed 120px to fit-content, with same !important weight. Also, the min-width and max-width properties are set to limit its size between 100px and 200px. With this change, the input placeholder (if limited to under 200px) will not get cropped, and it will also have a better fit into the input field, thereby offering it more flexibility to adapt the content.

Proposed changes

Basically, this change fixes the issue that I had encountered recently with the placeholder or text of the input before pressing enter to convert it to a chip. Please have a look at how this small change affects the chips input (Screenshot from Google Chrome):

Screenshots (if appropriate) or codepen:

Capture-ChangePropose

Types of changes

  • [ x ] Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to change).

Checklist:

  • [ x ] I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

This is a minor commit which updates the width of input field from fixed 120px to fit-content with same !important weight. Also, the optional min-width and max-width are set to limit its size between 100px and 200px. With this change, the input placeholder (if limited to under 200px) will not get cropped, and it will also have a better fit into the input field.
Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

Attention, this will not work on IE11. We should keep the old rule and add the new rule after it. So browsers who support it will use the second width rule.

https://caniuse.com/#feat=mdn-css_properties_width_fit-content

@Git-Harshit
Copy link
Author

Yes @DanielRuf sir! Thank you for mentioning! It did actually slip out of my mind when I had made this pull request. I think even the default 'auto' value for width property might not work in the desired way for IE.
Also, can you please tell if the !important weight is really necessary to be used here, as I don't think it is required for the width changes to take effect?

Now, I guess it's time to update this PR.

@DanielRuf
Copy link
Contributor

Regarding the new or old width setting? !important is bad in general.

@Git-Harshit
Copy link
Author

Regarding the new or old width setting? !important is bad in general.

Actually, it was about the old width setting, as there was !important used with width:120px CSS. Completely agreed, it isn't a good practice to use !important weight in general.

Still, as it was used in the previous code, I didn't remove it for now. Please let me know if in case it has to be discarded.

This commit updates input width in _chips.scss sass component from fixed 120px value to fit-content for browsers (wherever supported), so that the input width takes up just the required amount of width, and having it between `min-width:100px` and `max-width:200px` constraints ensures that the input field will always be within just the required width, and will be now be lesser likely to crop placeholders.
Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants