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

Dialog CSS/ Attribution link css: add z-index to dialogand remove from attribution links #1615

Merged

Conversation

AlexanderGeere
Copy link
Contributor

Description

Removed the z-index from the attribution-links class. Added a z-index of 5 to the dialog CSS.
The attribution links remain on top of everything, but dialogs can be moved over them.

GitHub Issue

#1614 and #1613

Type of Change

  • ✅ Bug fix (non-breaking change which fixes an issue)

How have you tested this?

Tested locally on /demo or any instance with a dialog.

Testing Checklist

  • ✅ Existing Tests still pass
  • ✅ Ran locally on my machine

Code Quality Checklist

  • ✅ My code follows the guidelines of XYZ
  • ✅ My code has been commented
  • ✅ Documentation has been updated
  • ✅ New and existing unit tests pass locally with my changes
  • ✅ Main has been merged into this PR

@AlexanderGeere AlexanderGeere added Bug A genuine bug. There must be some form of error exception to work with. CSS Changes in stylesheets v4.12.1 labels Oct 29, 2024
@AlexanderGeere AlexanderGeere self-assigned this Oct 29, 2024
This was linked to issues Oct 29, 2024
Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

I'd prefer if this can be solved without magic numbers.

@AlexanderGeere
Copy link
Contributor Author

I'd prefer if this can be solved without magic numbers.

5 puts the dialog above the sliders as they max out at 4

@dbauszus-glx
Copy link
Member

dbauszus-glx commented Oct 29, 2024

If god is 7 because the devil is 6, then that needs to be commented on because otherwise it is just a [magic] number.

And of course this needs to go back to the source... Why is the devil 6? Is he dependent on someone else being 5? Perhaps man.

At best it is confusing if z-indexes are defined accross the style sheets at worst there might be a cleaner solution which does not require the use of z-indexes in the first place. Perhaps the devil doesn't need to be 6.

RobAndrewHurst and others added 2 commits October 29, 2024 16:07
Copy link

Copy link
Contributor

@RobAndrewHurst RobAndrewHurst left a comment

Choose a reason for hiding this comment

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

@AlexanderGeere @dbauszus-glx

I have replace that z-index on the dialog with the css property isolation and giving it the value isolate which will create a new stack context on the dialog itself which will not create issues with other z-index's of other elements.

/* Keyword values */
isolation: auto;
isolation: isolate;

/* Global values */
isolation: inherit;
isolation: initial;
isolation: revert;
isolation: revert-layer;
isolation: unset;

We are going to want to use this to fix all of the z-index mess we have created in other styles :)

@RobAndrewHurst RobAndrewHurst dismissed dbauszus-glx’s stale review October 29, 2024 14:42

This has been fixed with the isolation property on the dialog

@simon-leech simon-leech changed the title Dialog CSS/ Attribtion link css: add z-index to dialogand remove from attribution links Dialog CSS/ Attribution link css: add z-index to dialogand remove from attribution links Oct 29, 2024
@RobAndrewHurst RobAndrewHurst merged commit 132cc8c into GEOLYTIX:main Oct 29, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A genuine bug. There must be some form of error exception to work with. CSS Changes in stylesheets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map attribution: Attribution links Dialog Positioning: Z-Index
4 participants