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

Fixes for positions of icon, text and upward sub-menu shadow and rounded border #2804

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 26 additions & 12 deletions src/definitions/collections/menu.less
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@

& when (@variationMenuVertical) {
.ui.vertical.menu .item > .header:not(.ui) {
flex: 0 0 100%;
margin: @verticalHeaderMargin;
font-size: @verticalHeaderFontSize;
font-weight: @verticalHeaderFontWeight;
Expand All @@ -200,7 +201,6 @@
/* Dropdown Icon */
.ui.menu .item > i.dropdown.icon {
padding: 0;
float: @dropdownIconFloat;
margin: 0 0 0 @dropdownIconDistance;
}

Expand All @@ -212,6 +212,10 @@
margin: @dropdownMenuDistance 0 0;
box-shadow: @dropdownMenuBoxShadow;
}
.ui.menu .upward.dropdown.item .menu {
border-radius: @dropdownMenuBorderRadius @dropdownMenuBorderRadius 0 0;
box-shadow: @dropdownUpwardMenuBoxShadow;
}
.ui.menu .dropdown.item:not(.column) .menu {
flex-direction: column;
}
Expand Down Expand Up @@ -244,14 +248,12 @@
}

.ui.menu .ui.dropdown.item .menu .item:not(.filtered) {
display: block;
display: flex;
}
.ui.menu .ui.dropdown .menu > .item > .icons,
.ui.menu .ui.dropdown .menu > .item > i.icon:not(.dropdown) {
display: inline-block;
font-size: @dropdownItemIconFontSize !important;
float: @dropdownItemIconFloat;
margin: @dropdownItemIconMargin !important;
order: 0;
}

& when (@variationMenuSecondary) or (@variationMenuText) {
Expand Down Expand Up @@ -279,9 +281,17 @@
& when (@variationMenuVertical) {
/* Vertical */
.ui.vertical.menu .dropdown.item > i.icon {
float: right;
content: "\f0da";
margin-left: 1em;
margin-left: auto;
order: @verticalIconOrder;
}
.ui.vertical.menu .dropdown.item > i.dropdown.icon {
margin-left: @dropdownIconDistance;
padding-left: 0;
order: 100;
}
.ui.vertical.menu .dropdown.item > i.dropdown.icon:only-of-type {
margin-left: auto;
}
.ui.vertical.menu .dropdown.item .menu {
left: 100%;
Expand All @@ -297,6 +307,7 @@
}
.ui.vertical.menu .dropdown.item.upward .menu {
bottom: 0;
top: auto !important;
Copy link
Member

Choose a reason for hiding this comment

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

Could !important get replaced by increasing .ui specificity?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are also !important flags that are not part of my PR which wouldn't allow to override with .ui specificity.

I would have to refactor and remove them too.

I feel like we've too many bloated CSS blocks in several modules that need to be removed.

Since we dropped the road map for FUI v3, we should consider to refactor this version to keep as much as clean and reduce the file size.

I would find the time for refactoring the CSS first. Then we can consider the refactoring JS modules.

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find any solution unless we wrap the text inside dropdown item with extra container like <span class="text">Item</span>.

Please check this fiddle: https://jsfiddle.net/ko2in/twpkudxs/1/

As you can see, it just work without any CSS modifications.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no way I could find to implement just alone CSS to make these icon positioning correctly unless the dropdown item text is warpped by another element.

This might be breaking change and not backward compatible and the users definitely need to migrate HTML structure.

Should I discard this PR or keep it for another major release? What do you think @lubber-de ?

Copy link
Member

Choose a reason for hiding this comment

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

Should I discard this PR or keep it for another major release? What do you think @lubber-de ?

Let's keep it (as your work is still very good!), as planned for 2.10, and take a look again later. I label that on hold until then and convert to draft.

}
.ui.vertical.menu .dropdown.item:not(.upward) .menu {
top: 0;
Expand Down Expand Up @@ -325,8 +336,7 @@
}
& when (@variationMenuVertical) {
.ui.vertical.menu .item > .label {
margin-top: @labelOffset;
margin-bottom: @labelOffset;
margin: @labelOffset 0 @labelOffset auto;
padding: @labelVerticalPadding @labelHorizontalPadding;
}
}
Expand Down Expand Up @@ -535,7 +545,9 @@ Floated Menu / Item

/* --- Item --- */
.ui.vertical.menu .item {
display: block;
display: flex;
flex-wrap: wrap;
align-items: center;
background: @verticalItemBackground;
border-top: none;
border-right: none;
Expand All @@ -549,15 +561,15 @@ Floated Menu / Item

/* --- Label --- */
.ui.vertical.menu .item > .label {
float: right;
order: 1;
text-align: center;
}

/* --- Icon --- */
.ui.vertical.menu .item > i.icon,
.ui.vertical.menu .item > i.icons {
order: @verticalIconOrder;
width: @iconWidth;
float: @verticalIconFloat;
margin: @verticalIconMargin;
}
.ui.vertical.menu .item > .label + i.icon {
Expand All @@ -582,6 +594,7 @@ Floated Menu / Item

/* --- Sub Menu --- */
.ui.vertical.menu .item > .menu {
flex: 0 0 100%;
margin: @subMenuMargin;
}
.ui.vertical.menu .menu .item {
Expand Down Expand Up @@ -1282,6 +1295,7 @@ Floated Menu / Item
display: block;
font-size: @labeledIconSize !important;
margin: 0 auto @labeledIconTextMargin !important;
order: 0;
}
& when (@variationMenuFluid) {
/* Fluid */
Expand Down
6 changes: 4 additions & 2 deletions src/definitions/modules/accordion.less
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,13 @@

/* Menu */
.ui.accordion.menu .item .title {
display: block;
display: flex;
flex: 0 0 100%;
justify-content: space-between;
padding: @menuTitlePadding;
}
.ui.accordion.menu .item .title > .dropdown.icon {
float: @menuIconFloat;
order: 2;
margin: @menuIconMargin;
transform: @menuIconTransform;
}
Expand Down
85 changes: 45 additions & 40 deletions src/definitions/modules/dropdown.less
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,12 @@
}
.ui.dropdown .menu > .item .dropdown.icon {
width: auto;
float: @itemDropdownIconFloat;
order: @itemDropdownIconOrder;
margin: @itemDropdownIconMargin;
}
.ui.dropdown .menu > .item .text {
flex-grow: 1;
}
.ui.dropdown .menu > .item .dropdown.icon + .text {
margin-right: @itemDropdownIconDistance;
}
Expand All @@ -99,8 +102,11 @@
--------------- */

.ui.dropdown > .text {
display: inline-block;
display: inline-flex;
order: 0;
transition: @textTransition;
align-items: baseline;
word-break: break-all;
}

/* --------------
Expand All @@ -110,7 +116,7 @@
.ui.dropdown .menu > .item {
position: relative;
cursor: pointer;
display: block;
display: flex;
border: @itemBorder;
height: @itemHeight;
min-height: @itemMinHeight;
Expand All @@ -123,6 +129,7 @@
text-transform: @itemTextTransform;
font-weight: @itemFontWeight;
box-shadow: @itemBoxShadow;
word-break: break-all;
-webkit-touch-callout: none;
}
.ui.dropdown .menu > .item:first-child {
Expand All @@ -131,24 +138,23 @@

.ui.dropdown .menu > .item.vertical {
display: flex;
flex-direction: column-reverse;
flex-direction: column;
}

/* --------------
Floated Content
--------------- */

.ui.dropdown > .text > [class*="right floated"],
.ui.dropdown .menu .item > [class*="right floated"] {
float: right !important;
margin-right: 0 !important;
margin-left: @floatedDistance !important;
.ui.ui.dropdown > .text > [class*="right floated"],
.ui.ui.dropdown .menu .item > [class*="right floated"] {
order: 1;
padding-left: @floatedDistance;
margin-left: auto;
}
.ui.dropdown > .text > [class*="left floated"],
.ui.dropdown .menu .item > [class*="left floated"] {
float: left !important;
margin-left: 0 !important;
margin-right: @floatedDistance !important;
.ui.ui.dropdown > .text > [class*="left floated"],
.ui.ui.dropdown .menu .item > [class*="left floated"] {
margin-left: 0;
margin-right: @floatedDistance;
}

.ui.dropdown .menu .item > i.icon.floated,
Expand Down Expand Up @@ -206,13 +212,15 @@

.ui.dropdown > .text > .description,
.ui.dropdown .menu > .item > .description {
float: @itemDescriptionFloat;
order: @itemDescriptionOrder;
margin: @itemDescriptionMargin;
padding: @itemDescriptionPadding;
color: @itemDescriptionColor;
}

.ui.dropdown .menu > .item.vertical > .description {
margin: 0;
padding: 0;
}

/* -----------------
Expand Down Expand Up @@ -287,8 +295,8 @@
.ui.dropdown .menu > .item > .flag,
.ui.dropdown .menu > .item > .image,
.ui.dropdown .menu > .item > img {
align-self: center;
margin-left: 0;
float: @itemElementFloat;
margin-right: @itemElementDistance;
}

Expand All @@ -300,8 +308,7 @@
.ui.dropdown > .text > .image:not(.icon),
.ui.dropdown .menu > .item > .image:not(.icon),
.ui.dropdown .menu > .item > img {
display: inline-block;
vertical-align: top;
align-self: center;
width: auto;
margin-top: @menuImageVerticalMargin;
margin-bottom: @menuImageVerticalMargin;
Expand Down Expand Up @@ -342,7 +349,7 @@

/* Dropdown Menu */
.ui.label.dropdown .menu {
min-width: 100%;
width: 100%;
}
}

Expand All @@ -356,7 +363,7 @@
margin: 0;
}
.ui.dropdown.button .menu {
min-width: 100%;
width: 100%;
}
.ui.dropdown.button:not(.pointing):not(.floating).active {
border-radius: @borderRadius @borderRadius 0 0;
Expand Down Expand Up @@ -785,7 +792,7 @@ select.ui.dropdown {
margin: @selectionIconMargin;
padding: @selectionIconPadding;
right: @clearableIconPosition;
top: @selectionVerticalPadding;
top: @clearableIconVerticalPosition;
position: absolute;
opacity: @clearableIconOpacity;
z-index: @selectionIconZIndex;
Expand Down Expand Up @@ -953,7 +960,7 @@ select.ui.dropdown {
}
.ui.inline.dropdown .dropdown.icon {
margin: @inlineIconMargin;
vertical-align: baseline;
vertical-align: middle;
}
.ui.inline.dropdown > .text {
font-weight: @inlineTextFontWeight;
Expand Down Expand Up @@ -1204,8 +1211,8 @@ select.ui.dropdown {
& when (@variationDropdownLeft) {
/* Leftward Opening Menu */
.ui.dropdown > .left.menu {
left: auto !important;
right: 0 !important;
left: auto;
right: 0;
}

.ui.dropdown > .left.menu .menu,
Expand All @@ -1218,20 +1225,10 @@ select.ui.dropdown {

.ui.dropdown .item .left.dropdown.icon,
.ui.dropdown .left.menu .item .dropdown.icon {
order: @leftMenuDropdownIconOrder;
width: auto;
float: @leftMenuDropdownIconFloat;
margin: @leftMenuDropdownIconMargin;
}
.ui.dropdown .item .left.dropdown.icon,
.ui.dropdown .left.menu .item .dropdown.icon {
width: auto;
float: @leftMenuDropdownIconFloat;
margin: @leftMenuDropdownIconMargin;
}
.ui.dropdown .item .left.dropdown.icon + .text,
.ui.dropdown .left.menu .item .dropdown.icon + .text {
margin-left: @itemDropdownIconDistance;
margin-right: 0;
padding-right: @itemDropdownIconDistance;
}
}

Expand Down Expand Up @@ -1480,6 +1477,15 @@ select.ui.dropdown {
margin-top: 0 !important;
}
& when (@variationDropdownUpward) {
.ui.simple.upward.active.dropdown > .menu,
.ui.simple.upward.dropdown:hover > .menu {
top: auto !important;
Copy link
Member

Choose a reason for hiding this comment

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

Can !important be replaced by .ui specificity?

}
.ui.simple.upward.dropdown > .menu > .item:active > .menu,
.ui.simple.upward.dropdown .menu .item:hover > .menu {
bottom: 0 !important;
top: auto !important;
Comment on lines +1486 to +1487
Copy link
Member

Choose a reason for hiding this comment

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

Can !important be replaced by .ui specificity?

}
.ui.simple.dropdown .upward.menu {
margin-bottom: @simpleUpwardMenuMargin;
}
Expand All @@ -1500,15 +1506,15 @@ select.ui.dropdown {
.ui.simple.active.dropdown > .menu,
.ui.simple.dropdown:hover > .menu {
overflow: visible;
width: auto;
width: 100%;
height: auto;
top: 100%;
opacity: 1;
}
.ui.simple.dropdown > .menu > .item:active > .menu,
.ui.simple.dropdown .menu .item:hover > .menu {
overflow: visible;
width: auto;
width: 100%;
height: auto;
top: 0 !important;
left: 100%;
Expand Down Expand Up @@ -1574,8 +1580,7 @@ select.ui.dropdown {
Floating
--------------- */

.ui.floating.dropdown > .menu {
left: 0;
.ui.floating.dropdown .menu {
Copy link
Member

Choose a reason for hiding this comment

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

The direct child selector > was introduced by #2284
This change re-introduces that behavior again (see your jsfiddle)
image

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see that in my fiddle and local version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry. You're right. I was checking the dropdown without scrollbar.

right: auto;
box-shadow: @floatingMenuBoxShadow !important;
border-radius: @floatingMenuBorderRadius !important;
Expand Down
6 changes: 3 additions & 3 deletions src/themes/default/collections/menu.variables
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@
@iconOpacity: 0.9;

/* Dropdown Icon */
@dropdownIconFloat: right;
@dropdownIconDistance: 1em;

/* Header */
Expand All @@ -67,8 +66,8 @@
@headerTextTransform: @normal;

/* Vertical Icon */
@verticalIconFloat: right;
@verticalIconMargin: 0 0 0 0.5em;
@verticalIconOrder: 2;
@verticalIconMargin: 0 0 0 auto;

/* Vertical Header */
@verticalHeaderMargin: 0 0 0.5em;
Expand Down Expand Up @@ -158,6 +157,7 @@
@secondaryDropdownMenuDistance: @relative5px;
@pointingDropdownMenuDistance: 0.75em;
@invertedSelectionDropdownColor: @invertedTextColor;
@dropdownUpwardMenuBoxShadow: 0 0 3px 0 rgba(0, 0, 0, 0.08);

/* --------------
States
Expand Down
Loading