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

Conversation

ko2in
Copy link
Member

@ko2in ko2in commented May 27, 2023

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
dropdown-remove-icon-not-aligned-properly

After
dropdown-remove-icon-aligend-properly

Sub-menu position in floating menu item

The sub-menu of dropdown item was not positioned correctly when the dropdown has floating variant. It was overlapping it's menu item.

Before
floating-left-sub-menu-bug

After
left-floating-sub-menu

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
upward-incorrect-shadow-and-rounded-border

After
upward-correct-shadow-and-rounded-border

Testcase

https://jsfiddle.net/ko2in/4f3xh2sd/

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.
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

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/
image

After

Your code
https://jsfiddle.net/lubber/fs3yom7t/2/
image

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;
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.

@@ -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?

Comment on lines +1486 to +1487
bottom: 0 !important;
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?

@@ -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.

@lubber-de lubber-de added lang/css Anything involving CSS state/awaiting-reviews Pull requests which are waiting for reviews tag/breaking-change Any pull request which is waiting for a breaking change release type/bug Any issue which is a bug or PR which fixes a bug labels May 27, 2023
@lubber-de lubber-de added this to the 2.10.x milestone May 27, 2023
@lubber-de lubber-de added the state/on-hold Issues and pull requests which are on hold for any reason label Sep 14, 2023
@lubber-de lubber-de marked this pull request as draft September 14, 2023 19:04
@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/css Anything involving CSS state/on-hold Issues and pull requests which are on hold for any reason tag/breaking-change Any pull request which is waiting for a breaking change release type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants