Skip to content

Comments

Wrap EuiHorizontalStep titles instead of truncating them.#653

Merged
cjcenizal merged 4 commits intoelastic:masterfrom
cjcenizal:wrap-horizontal-step-label
Apr 11, 2018
Merged

Wrap EuiHorizontalStep titles instead of truncating them.#653
cjcenizal merged 4 commits intoelastic:masterfrom
cjcenizal:wrap-horizontal-step-label

Conversation

@cjcenizal
Copy link
Contributor

I think we can handle variable-length text more gracefully by wrapping it instead of truncating it. This way the text is still legible and the UI remains usable. As consumers we'll have to be careful about choosing copy which isn't unreasonably long.

image

@cjcenizal cjcenizal force-pushed the wrap-horizontal-step-label branch from d62c87a to 8a9fcfc Compare April 10, 2018 23:15
@cjcenizal cjcenizal requested review from cchaos and snide April 10, 2018 23:15
@cjcenizal cjcenizal force-pushed the wrap-horizontal-step-label branch 3 times, most recently from 82b0ba0 to dc72ce7 Compare April 11, 2018 02:57
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.

Yeah I guess you're right... ;)

Though there is still an issue with the disabled version allowing selection and tabbing.

display: flex;
flex-direction: column;
align-items: center;
justify-content: flex-start;
Copy link
Contributor

Choose a reason for hiding this comment

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

All that's needed here is display: flex and I'm not sure I remember why the flex-grow or flex-basis was added to this high level, but I think you can remove it here now that it's display: flex.

@cjcenizal
Copy link
Contributor Author

@cchaos I was pondering the question of whether disabled elements should be focusable/tabbable and came upon this discussion which clarified things for me: https://ux.stackexchange.com/questions/103239/should-disabled-elements-be-focusable-for-accessibility-purposes?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa. Basically, if an element is disabled but visible then it also needs to be focusable -- if it's only visible but not focusable then sighted users can easily find it but people using screen readers will often never discover it. So the conclusion is to always make disabled elements tabbable and focusable. WDYT of using this as a general accessibility rule of thumb?

I agree that we should block the click on that disabled step though. Good catch. I'll also look into the flex thing, I think there may be a reason for those styles. If there is then I'll add a comment, otherwise I'll remove them. Thanks for the review!

@cchaos
Copy link
Contributor

cchaos commented Apr 11, 2018

I can somewhat understand that. The one concern then is that browsers treat buttons that are :disabled as though they are not focusable. To me, I'd rather stick to how browser's have been supporting this property as, I would think, users are used to this behavior and it's a known paradigm.

@cjcenizal
Copy link
Contributor Author

That's a good point, it would be more consistent so it's probably the safer choice. I'll change the disabled step so it can't receive focus. @aphelionz Can you provide some guidance here in the meantime?

@cjcenizal cjcenizal force-pushed the wrap-horizontal-step-label branch from dc72ce7 to 5d72434 Compare April 11, 2018 19:02
@cjcenizal
Copy link
Contributor Author

@cchaos I addressed your feedback, could you take another look?

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! LGTM

@cjcenizal cjcenizal force-pushed the wrap-horizontal-step-label branch from 5d72434 to 3753dc4 Compare April 11, 2018 20:33
@cjcenizal cjcenizal merged commit 2785f19 into elastic:master Apr 11, 2018
@cjcenizal cjcenizal deleted the wrap-horizontal-step-label branch April 11, 2018 20:34
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