Fixes for positions of icon, text and upward sub-menu shadow and rounded border #2804
Conversation
Remove left padding from dropdown icon. Instead add right padding to the dropdown item text since the icon display next to the text when sub menu opens to the right direction as default. Specify different shadow and rounded border for upward menu. Increase the amount of top position value of remove icon of dropdown to shift down to get better alignment along with the text and dropdown icon. In previous version, the remove icon placed a few pixels above from the text and dropdown icon which causes the inappropriate alignment. Removed vertical alignment from the text container which causes incorrect alignment with the elements around when it contains the content which takes more space vertically such as an image. Instead, use `align-item` property on the text container to have everything inside of it in a linear position. Remove unnecessary overrides and left position from menu which causes the sub-menu positioned incorrectly when it's inside floating dropdown item. In vertical menu, the icon should be displayed above the text, so the lowest order number should be specified.
There was a problem hiding this comment.
The example from the previous PR does not work anymore. The submenu icon is misplaced and the text is not left aligned anymore
Before
Your jsfiddle using FUI 2.9.2 without your changes
https://jsfiddle.net/lubber/8Ly7x2ju/1/
After
Your code
https://jsfiddle.net/lubber/fs3yom7t/2/
I'll tag this as breaking change for 2.10.x as you removed/renamed LESS variables, 2.10.x will drop IE support and i think this needs more testing, especially because of the change to flexbox and order.
Please also see my other comments
| @@ -297,6 +307,7 @@ | |||
| } | |||
| .ui.vertical.menu .dropdown.item.upward .menu { | |||
| bottom: 0; | |||
| top: auto !important; | |||
There was a problem hiding this comment.
Could !important get replaced by increasing .ui specificity?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
| @@ -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; | |||
There was a problem hiding this comment.
Can !important be replaced by .ui specificity?
| bottom: 0 !important; | ||
| top: auto !important; |
There was a problem hiding this comment.
Can !important be replaced by .ui specificity?
| @@ -1574,8 +1580,7 @@ select.ui.dropdown { | |||
| Floating | |||
| --------------- */ | |||
|
|
|||
| .ui.floating.dropdown > .menu { | |||
| left: 0; | |||
| .ui.floating.dropdown .menu { | |||
There was a problem hiding this comment.
The direct child selector > was introduced by #2284
This change re-introduces that behavior again (see your jsfiddle)
There was a problem hiding this comment.
I don't see that in my fiddle and local version.
There was a problem hiding this comment.
Sorry. You're right. I was checking the dropdown without scrollbar.
lubber-de
added
lang/css
lubber-de
added
the
state/on-hold
lubber-de
removed
the
state/awaiting-reviews
Description
This PR is a rebasing from my other PR which messed up with merging that resulted the dist directory included.
Remove icon alignment
The remove icon wasn't aligned in the best position as it was shifted above comparing the text and dropdown icon.
Before
After
Sub-menu position in floating menu item
The sub-menu of dropdown item was not positioned correctly when the dropdown has
floatingvariant. It was overlapping it's menu item.Before
After
Incorrect rounded corners and shadows of simple dropdown with upward direction
The rounded corners of upward simple dropdown menu should be top-left and top-right and the shadow should be displayed above the menu. That is fixed in this commit.
Before
After
Testcase
https://jsfiddle.net/ko2in/4f3xh2sd/