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

New design pattern updates for menu and listbox #78

Merged
merged 5 commits into from
Mar 6, 2018
Merged

New design pattern updates for menu and listbox #78

merged 5 commits into from
Mar 6, 2018

Conversation

seangates
Copy link
Contributor

Closes #62

<div class="demo">
<div class="demo__inner">
<span class="menu">
<button class="expand-btn btn--borderless" aria-haspopup="true" type="button">
Copy link
Contributor

@ianmcburnie ianmcburnie Mar 4, 2018

Choose a reason for hiding this comment

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

The base class is expand-btn, so it's modifier has to be expand-btn--borderless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know, I actually had it that way at the very beginning. I’ll adjust.

@seangates
Copy link
Contributor Author

@ianmcburnie Please note that there were a couple other changes that I noticed in the DSL spec that I have fixed in these most recents changes (after your PR comment).

Let me know if you have any questions about them.

@ianmcburnie
Copy link
Contributor

Thanks, will do another look through ASAP.

Copy link
Contributor

@ianmcburnie ianmcburnie left a comment

Choose a reason for hiding this comment

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

Looking good. I'm considering merging this into 3.4.0 to coincide with 0.1.0 of coreui...

@@ -82,13 +93,13 @@ div.menu__item[role^="menuitem"]:hover,
div.listbox__option[role^="option"]:hover,
a.fake-menu__item:hover,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this background color for :focus too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you expand the lines right above this, it's all part of the same group of selectors (focus and hover).

Copy link
Contributor

@ianmcburnie ianmcburnie Mar 5, 2018

Choose a reason for hiding this comment

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

My bad. Of course the items themselves never gain focus, so I guess this bg color will come from aria-selected instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, I moved this around a tiny bit, in order to fix the linting complaining, but it is accomplished exactly the same.

@@ -15,6 +15,18 @@ span.listbox {

// BEM elements

.menu button.expand-btn--borderless,
.fake-menu button.expand-btn--borderless {
border: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Lesshint is throwing a warning about border: none.

Warning: dropdown-base.less: line 20, col 13, borderZero: Border properties should use 0 instead of "none".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, let me grab that one. Old habits die hard ...

}
div.menu__item[role^="menuitem"]:focus,
div.listbox__option[role^="option"]:focus,
a.fake-menu__item:focus,
button.fake-menu__item:focus {
outline: 1px solid #c7c7c7;
outline: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Csshint is throwing a warning here:

Outlines shouldn't be hidden unless other visual changes are made.
div.menu__item[role^="menuitem"]:focus

I guess we can overrule this line, as we are making other visual changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do we overrule it when comments are stripped out?

@ianmcburnie ianmcburnie changed the title New design patterns for "dropdown", a.k.a. menu/select (soon-to-be-formerly listbox) New design pattern updates for menu and listbox Mar 5, 2018
@ianmcburnie ianmcburnie changed the base branch from 3.5.0 to 3.4.0 March 6, 2018 00:01
@seangates seangates merged commit 213288e into 3.4.0 Mar 6, 2018
@seangates seangates deleted the 62 branch March 6, 2018 00:07
@seangates seangates mentioned this pull request Mar 6, 2018
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