[UI Framework] Change menu to unordered list #12102
Conversation
rhoboat
added
WIP
There was a problem hiding this comment.
Looks good in the browser! I really like this change. Thanks for doing it! Just had a few comments.
Also, I think we generally hold off on creating backport PRs until the PR into master has been merged. It's a bit easier that way because you don't have to worry about changes made in response to code reviews of the original PR rippling throughout the backport PRs. And you also won't need to get the backport PRs reviewed 99% of the time, because they code won't be any different than the original.
| line-height: $globalLineHeight; | ||
| color: $globalFontColor; | ||
| } | ||
| .kuiEventBody__message { |
There was a problem hiding this comment.
Can we keep these indented? See https://github.com/elastic/kibana/blob/master/style_guides/css_style_guide.md#create-descendant-classes-to-represent-child-components-which-cant-stand-on-their-own for more context.
| */ | ||
| .kuiMenu { | ||
| padding-left: 0; | ||
| } |
There was a problem hiding this comment.
Can we structure the comment like this? This will be more consistent with the rest of the comments in the UI Framework.
/**
* 1. Allow this class to be applied to `ol` and `ul` elements.
*/
.kuiMenu {
padding-left: 0; /* 1 */
}| .kuiMenu--contained { | ||
| border: $globalBorderThin; | ||
| padding-left: 0; |
There was a problem hiding this comment.
This isn't necessary because kuiMenu--contained should only be applied to elements that also have kuiMenu applied to it.
|
|
||
| .kuiMenuItem { | ||
| padding: 6px 10px; | ||
| } | ||
| } | ||
|
|
||
| .kuiMenuItem { | ||
| list-style: none; |
There was a problem hiding this comment.
And this would become:
/**
* 1. Allow this class to be applied to `li` elements.
*/
.kuiMenuItem {
list-style: none; /* 1 */
}
|
@cjcenizal Implemented your requests. |
|
I'm going to go ahead and backport changes because I have a dependent feature on 5.x currently. I probably should have built my other feature on master, but that feature has the most changes for 5.x rather than master, since it's our migration assistant. If I don't backport I'm basically unable to do much work. These are small enough changes that I feel ok about that extra work. |
|
|
||
| .kuiMenuItem { | ||
| padding: 6px 10px; | ||
| } | ||
| } | ||
|
|
||
| .kuiMenuItem { | ||
| list-style: none; | ||
| list-style: none; /* 2 */ |
There was a problem hiding this comment.
Can we scope this comment to the class itself, like in my original comment? As we add more comments, they scale better when the comments are localized like this.
There was a problem hiding this comment.
Your original comment had the /* 1 */ on the same line as the rule. What am I not seeing?
There was a problem hiding this comment.
I meant like this:
/**
* 1. Allow this class to be applied to `li` elements.
*/
.kuiMenuItem {
list-style: none; /* 1 */
}There was a problem hiding this comment.
Do you mean the block comment that explains what each number is?
There was a problem hiding this comment.
Yep, that block comment is now local to the class to which it pertains.
There was a problem hiding this comment.
Gotcha. Once I do that, ok to merge? I don't think this needs a 2nd reviewer.
|
Might want to rebase master though, to get the tests to pass. |
In some instances, a Menu as an unordered list feels more semantically accurate than using divs. Correspondingly MenuItems are list items.