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

A few quick fixes #2585

Merged
merged 11 commits into from
Dec 3, 2019
Merged

A few quick fixes #2585

merged 11 commits into from
Dec 3, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Dec 2, 2019

EuiSwitch

Replaced <p> tag surrounding the label with a <span> tag.

There were instances where the switch could be a child of EuiText which would then add margin to the bottom of p tag and cause alignment issues. Instead of trying to override any unwanted styles applied to p tags because of EuiText, it was easiest just to replace with a span which is also block-level more similar to a label which is what it's replacing.

Before
Screen Shot 2019-11-28 at 1 31 47 PM

After
Screen Shot 2019-11-28 at 1 32 33 PM

Fixed layout if the label wrapped to more lines

Using display: inline-flex this keeps the label in its own column and keeps the toggle at the top similar to the wrapping of checkboxes. IE lays it out fine too.

Before
Screen Shot 2019-12-02 at 12 24 00 PM

After
Screen Shot 2019-12-02 at 12 23 02 PM
Screen Shot 2019-12-02 at 12 25 23 PM

Question

There's a new prop for EuiFormRow that is called hasChildLabel. What it does is removes the for attribute from the label so that when used with EuiSwitch, there isn't two labels associated with the switch. However, as I was trying to understand the prop, I think the value is set backwards.

I would think that hasChildLabel = true means that the form row's child has a label so it should remove the form row's reference. When in fact hasChildLabel = false will remove the reference.

@myasonik Maybe you can shed some light? I would assume when adding a switch which has a label, one would add hasChildLabel = true to the EuiFormRow.

EuiSelectable

Add the same padding from EuiSelectableListItem to the header

I noticed in a Maps configuration where they had EuiSelectable with headers inside a popover, that the header was far left and butting right up against the popover edge.

Screen Shot 2019-12-02 at 18 54 39 PM

This PR just adds the same full padding to the whole heading as it does the item so they line up correctly.

Screen Shot 2019-12-02 at 18 09 01 PM

Screen Shot 2019-12-02 at 18 09 25 PM

Checklist

  • Checked in dark mode
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • [ ] Added documentation 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

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Tested locally and everything looks good! 👍

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

EuiSwitch changes LGTM assuming there was no a11y reason <p> was used originally.
@myasonik would have the answer

@myasonik
Copy link
Contributor

myasonik commented Dec 3, 2019

I would assume when adding a switch which has a label, one would add hasChildLabel = true to the EuiFormRow.

Yeah, that does sound backwards. My bad... Potentially bigger than you want to tackle in this PR but I also wrote up #2493 a little while ago because I think the whole API could be improved/cleaned up a little more.

@cchaos
Copy link
Contributor Author

cchaos commented Dec 3, 2019

Thanks @myasonik , yeah I just wanted to make sure I was understanding correctly. I'll keep that for a later date. What are your thoughts on the p vs span. Do you think there's an a11y issue with changing to a span?

@myasonik
Copy link
Contributor

myasonik commented Dec 3, 2019

What are your thoughts on the p vs span. Do you think there's an a11y issue with changing to a span?

Technically, yes. p has semantic meaning ("inside me, there is text") whereas a span doesn't ("inside me, there could be anything!")...

But... I wasn't going to say anything because I don't think there's a meaningful, practical impact with the current situation. With the aria-labelledby still hooked up, I can't imagine a use case where this would be a problem. (That's not to say it doesn't exist, but I don't know what it is.)

@cchaos
Copy link
Contributor Author

cchaos commented Dec 3, 2019

Great, thanks for the explanation. We can revisit a p if need be.

@cchaos
Copy link
Contributor Author

cchaos commented Dec 3, 2019

Jenkins, test this

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.

5 participants