-
Notifications
You must be signed in to change notification settings - Fork 58
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
Components/dropdown #1065
Components/dropdown #1065
Conversation
html[dir="rtl"] &.is-active, html[dir="rtl"] &:focus { | ||
border-left: none; | ||
border-right: 3px solid $blue; | ||
html[dir="rtl"] &.is-active:before, html[dir="rtl"] &:focus:before, html[dir="rtl"] &:hover:before { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need all these selectors. You're only declaring the left
value on the :before
pseudo element above, so that's the only one you need to correct in right to left. Just using
html[dir="rtl"] &:before {
left: unset;
right: 10px;
}
should be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, done.
border-right: 3px solid $blue; | ||
html[dir="rtl"] &.is-active:before, html[dir="rtl"] &:focus:before, html[dir="rtl"] &:hover:before { | ||
right: 10px; | ||
padding-right: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the :before
doesn't have any padding, so I don't think you need this either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
padding-left: 0; | ||
&:hover { | ||
background: $white; | ||
&:before { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly I wonder if it would be easier to not inherit from dropdown link so you don't have to unset all of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it
@@ -68,6 +89,7 @@ | |||
z-index: 1000; | |||
display: none; | |||
overflow: auto; | |||
padding: 20px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible we should probably use rem
or baseline
units here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to rem
or baseline
cursor: auto; | ||
border-bottom: 1px solid $lightest-gray; | ||
margin: $baseline/3 $baseline/1.5 0 $baseline/1.5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behind door number 3.... Making the first-child link have a transparent 1px border that on hover becomes #eaeaea to match the header-bottom. That way the border beneath the headline is maintained but there is no staggering:
Couple other styling things:
Could we maybe tighten up the space between items by reducing y-padding 2-3 px ish? I think that would put it more in line with what's on DevExt now.
I think the active link and the header text are good candidates for 500
weight.
Cool with no animation / fade in or out for hover state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will work on these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated spacing on title and links. Also added a fade in transition $transition
@@ -12,59 +12,79 @@ | |||
} | |||
} | |||
|
|||
%dot { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you know, now that I'm looking at it, I think that these classes are global to sass, so we might want to name this something a little more specific as it's pretty likely somebody someday will have a sass class called dot
. Maybe active-dot
or dropdown-dot
. IDK. Maybe @macandcheese will have a good idea on the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, either of those make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%dot is really only used here in one place, it might be useful to define it in a utils
file to re-use the style in other upcoming updates, like side nav, or filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, that's what I was thinking. @paulcpederson, you good with this?
Also, any thoughts on the class name? is dot
good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do like active-link-dot
or something more specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, few things.
} | ||
|
||
@mixin dropdown-btn { | ||
cursor: pointer; | ||
position: relative; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anymore? If not, let's remove from here and the include at the end of the file.
border-right: 3px solid $blue; | ||
html[dir="rtl"] &:before { | ||
left: unset; | ||
right: .6rem; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these be included in %dot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -12,59 +12,79 @@ | |||
} | |||
} | |||
|
|||
%dot { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%dot is really only used here in one place, it might be useful to define it in a utils
file to re-use the style in other upcoming updates, like side nav, or filters.
I think aligned with top line is good. Centered in element falls between the two lines of text and looks less intentional, IMO |
@@ -8,66 +8,50 @@ | |||
position: relative; | |||
display: inline-block; | |||
&.is-active .dropdown-menu { | |||
display: block; | |||
opacity: 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you generally can't use opacity alone to do UI's like this. The reason being I can't click anything behind the dropdown, even when it's closed. Also, you can tab into the hidden menu and focus on the links that are invisible.
If we want to do an animation, I would suggest using visibility: hidden
and opacity together. There is a specific way you can combine them to get the animation and still have normal hiding. We do it in modals:
https://github.com/Esri/calcite-web/blob/master/lib/sass/calcite-web/patterns/_modal.scss#L21-L36
If we go that direction, a small transition down during the fade in would help relate the menu with the dropdown via the animation
- Added gray dots on hover - active state (click) hets blue dot - for some reason position was off, added top: 1 rem
Closing, will re-open fresh PR. |
Updated dropdowns UI (styles)
from #1021