-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Site Editor: Inline layout z-index values #77807
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,7 +23,7 @@ | |||||||||
| } | ||||||||||
|
|
||||||||||
| .edit-site-layout__sidebar-region { | ||||||||||
| z-index: z-index(".edit-site-layout__sidebar"); | ||||||||||
| z-index: 1; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the refactor, it becomes harder to understand the context behind these values, which may cause future unwated breakage. IMO we should either add CSS comments around the file, eg
Suggested change
Or group those
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest I'm finding it hard to do so when the intent behind the values have never been written down anywhere 😅 Are our comments going to capture some of the original intent, but not the whole intent? We're adding a layer of possible inaccuracy by adding assumptive comments… There have also been a handful of places where I'm actually not sure what the z-index is even for (i.e. removing it doesn't seem to cause immediate problems).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could at least mention the elements / selectors to which each z-index is related? Up to you, not a strong opinion
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried my best but I could not figure out why any of these z-indexes are needed 💀 Disabling them doesn't trigger any obvious changes. |
||||||||||
| width: 100vw; | ||||||||||
| flex-shrink: 0; | ||||||||||
|
|
||||||||||
|
|
@@ -60,7 +60,7 @@ | |||||||||
| .edit-site-layout__mobile { | ||||||||||
| position: relative; | ||||||||||
| width: 100%; | ||||||||||
| z-index: z-index(".edit-site-layout__canvas-container"); | ||||||||||
| z-index: 2; | ||||||||||
| display: flex; | ||||||||||
| flex-direction: column; | ||||||||||
|
|
||||||||||
|
|
@@ -84,7 +84,7 @@ | |||||||||
| .edit-site-layout__canvas-container { | ||||||||||
| position: relative; | ||||||||||
| flex-grow: 1; | ||||||||||
| z-index: z-index(".edit-site-layout__canvas-container"); | ||||||||||
| z-index: 2; | ||||||||||
| // When animating the frame size can exceed its container size. | ||||||||||
| overflow: visible; | ||||||||||
|
|
||||||||||
|
|
@@ -97,7 +97,7 @@ | |||||||||
| right: 0; | ||||||||||
| bottom: 0; | ||||||||||
| content: ""; | ||||||||||
| z-index: z-index(".edit-site-layout__canvas-container.is-resizing::after"); | ||||||||||
| z-index: 100; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noting that this could probably be just any positive number, but I understand that we may keep it at |
||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a new package release happened after opening this PR — we'll need to rebase/merge trunk and create a new CHANGELOG entry in the Unreleased section