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

[EuiComboBox] Truncate long placeholder text with ellipsis #4210

Merged
merged 6 commits into from
Nov 9, 2020

Conversation

banderror
Copy link
Contributor

@banderror banderror commented Nov 2, 2020

Summary

Addresses the following issue in the security_solution plugin's UI: elastic/kibana#81284
Basically there's a bunch of comboboxes which have long placeholders, and here's how it looks like:

I added a quick fix: truncation with CSS.

Screenshots

http://localhost:8030/#/forms/combo-box

Before:

Screenshot 2020-11-02 at 16 11 40

After:

Screenshot 2020-11-02 at 15 53 54

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • [ ] Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • [ ] Props have proper autodocs
  • [ ] Added documentation
  • [ ] Checked Code Sandbox works for the any docs examples
  • [ ] Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4210/

@cchaos cchaos self-requested a review November 2, 2020 20:32
Copy link
Contributor

@cchaos cchaos 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 getting this one started @banderror !

We do actually have some SASS utility mixins to help with both adding the appropriate padding to accommodate for the arrow (instead of hard-coded px values) and truncation. I've pushed a commit for you.

We also needed to account for when the component was loading too, so I used the same mixin for those states.

Since this is style only, there's only a few checklist items to perform (I'll cross out the ones that don't apply). The main one is to add a Changelog entry. See the file for formatting. You can put this one under the Bug Fixes header and be sure to use past tense.

@cchaos
Copy link
Contributor

cchaos commented Nov 2, 2020

Another solution to your placeholder issue is to remove the "Please" intro in the placeholder. That'll make the text more succinct.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4210/

@banderror banderror force-pushed the fix-combobox-placeholder branch from f0cb248 to b10650f Compare November 4, 2020 14:19
@banderror banderror marked this pull request as ready for review November 4, 2020 14:21
@banderror banderror requested a review from cchaos November 4, 2020 14:21
@banderror
Copy link
Contributor Author

@cchaos I addressed all your suggestions, please take a look. Thank you for your help, super appreciate it!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4210/

Copy link
Contributor

@cchaos cchaos 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 for the PR. Just one last quick change to the CL but the rest is good!

CHANGELOG.md Outdated Show resolved Hide resolved
@banderror banderror force-pushed the fix-combobox-placeholder branch from fb147b5 to 5a32d33 Compare November 9, 2020 17:40
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4210/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4210/

@cchaos cchaos merged commit fd8bf9b into elastic:master Nov 9, 2020
@banderror banderror deleted the fix-combobox-placeholder branch November 10, 2020 11:00
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.

3 participants