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

BUGFIX: Prevent hideDrawer call on hovering menu #3289

Merged
merged 3 commits into from Dec 5, 2022

Conversation

markusguenther
Copy link
Member

@markusguenther markusguenther commented Dec 4, 2022

Since 8.2 the drawer closes while hovering the menu items. We had some issues with the focus shift over all layers.
Because more or less everything was with z-index 1 in that case.

What I did
The drawer has now a higher z-index. The already existing constants have no more space between. But did not touch the most context bounded constants.

The text of the button now also fills the whole existing space.

How to verify it

Open the drawer menu and hover slowly over the menu items, especially where no test is.

Fixes: #3286

@markusguenther markusguenther added Bug Label to mark the change as bugfix 8.2 labels Dec 4, 2022
@mhsdesign
Copy link
Member

Do you know how this bug was introduced?

And i dont like that the ui constants now differ from their original origin in Neos.Neos https://github.com/neos/neos-development-collection/blob/3d2ddf4a8191aafe8ac536092c9fb64bd8d6a45c/Neos.Neos/Resources/Private/Styles/_CSSVariables.scss#L57

@Sebobo
Copy link
Member

Sebobo commented Dec 5, 2022

And i dont like that the ui constants now differ from their original origin in Neos.Neos https://github.com/neos/neos-development-collection/blob/3d2ddf4a8191aafe8ac536092c9fb64bd8d6a45c/Neos.Neos/Resources/Private/Styles/_CSSVariables.scss#L57

They need to be removed in 9.0 from Neos.Neos IMO.

@Sebobo
Copy link
Member

Sebobo commented Dec 5, 2022

I would like to have a proper system for the z-indices. But as those are currently internal variables, I think we don't have to worry about changing them again at some point?

@crydotsnake crydotsnake self-requested a review December 5, 2022 10:27
@crydotsnake crydotsnake linked an issue Dec 5, 2022 that may be closed by this pull request
@mhsdesign
Copy link
Member

The variables are exposed via react-ui-components postcss.rc, but fine with me - no one should use these vars besides us :P
So lets review this based on if its working or not ^^

We still have no clue about this no?

Do you know how this bug was introduced?

Im just curious because the fix is a bit magic to me

Copy link
Member

@crydotsnake crydotsnake left a comment

Choose a reason for hiding this comment

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

Tested it, and from the functionality its working.

@crydotsnake
Copy link
Member

Do you know how this bug was introduced?

The commit history should give some answers maybe 🤔

It COULD also be that you tried to fix another problem at a different location, and that's how this error came about 🤷🏽.

@Sebobo
Copy link
Member

Sebobo commented Dec 5, 2022

Most CSS problems came from the different behaviour how the old build system composed extended styles. Markus knows the details as he fixed a few of them already.

@markusguenther
Copy link
Member Author

I guess it is complicated to find the real root cause in this case. Guess it is a combination of some changes.
What Sebastian means is that we used composes in the past and that the current way of include for instance the resets styles and then override some of them by use case behave not always the same.

Because in composes, we create a subset of styles and don't overwrite.
The C from CSS is sometimes biting us. But the cascade is also the powerful part of CSS.

@markusguenther
Copy link
Member Author

I would like to have a proper system for the z-indices. But as those are currently internal variables, I think we don't have to worry about changing them again at some point?

That would be awesome. Not so many elements that all use z index 1, and then you have the issues when you need to fix something or extend something.

@mhsdesign
Copy link
Member

a i see f22441d was a regression too ;) im just trying to keep track of the things that broke, so in the case we go back to composes we would know what to adjust... but it doesnt seem like a good idea to ever do that? 😅

@markusguenther
Copy link
Member Author

Do you know how this bug was introduced?

And i dont like that the ui constants now differ from their original origin in Neos.Neos https://github.com/neos/neos-development-collection/blob/3d2ddf4a8191aafe8ac536092c9fb64bd8d6a45c/Neos.Neos/Resources/Private/Styles/_CSSVariables.scss#L57

Can open a PR and can adopt them there as well... to be in sync

markusguenther added a commit to markusguenther/neos-development-collection that referenced this pull request Dec 5, 2022
In the neos-ui we are adopting some z-index values to have more flexibility. To be in sync we should changed it here as well.

@see neos/neos-ui#3289
@markusguenther
Copy link
Member Author

@mhsdesign synced the z-index values neos/neos-development-collection#3975

@markusguenther markusguenther merged commit 9d4a6a9 into neos:8.2 Dec 5, 2022
markusguenther added a commit to markusguenther/neos-development-collection that referenced this pull request Dec 5, 2022
In the neos-ui we are adopting some z-index values to have more flexibility. To be in sync we should changed it here as well.

@see neos/neos-ui#3289
grebaldi pushed a commit to grebaldi/PackageFactory.Guevara that referenced this pull request Jan 10, 2023
* TASK: Give z-index constants more space

* BUGFIX: Prevent closing drawer while hovering menu items

* TASK: Fix codestyle mentions
neos-bot pushed a commit to neos/neos that referenced this pull request Apr 9, 2023
In the neos-ui we are adopting some z-index values to have more flexibility. To be in sync we should changed it here as well.

@see neos/neos-ui#3289
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.2 Bug Label to mark the change as bugfix
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Flyout menu closes
4 participants