Edit Site: Fix site editor canvas edit mode button#53730
Merged
Conversation
tyxla
added
[Type] Bug
tyxla
requested review from
Mamaduka,
flootr,
jsnajdr,
ntsekouras and
youknowriad
|
Size Change: +7 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
SiobhyB
added
the
[Feature] Site Editor
youknowriad
reviewed
Aug 16, 2023
| display: flex; | ||
| position: absolute; | ||
| top: 0; | ||
| left: 0; |
Contributor
There was a problem hiding this comment.
Nit: IMO, a component (any component) shouldn't have position absolute because that's a position that is relative to a container, instead I think this kind of style should be moved to the component that uses this one.
Member
Author
There was a problem hiding this comment.
That's an interesting opinion, thanks for sharing!
I guess one could argue that the width and height should also be defined relative to the parent container, the opacity, or even a few more of the properties. With that in mind, how do we decide where to place each one of them?
IMHO the way this component is designed to work right now, it will always take the full width and height, and it only makes sense to be absolute so it doesn't prevent anything else from being displayed where we need inside the canvas.
From my perspective, we're using this component only for the loading state of the site editor, and it's a special version of the loading spinner/progress bar that's intended specifically for the site editor. Therefore, in this single place, it makes the most sense to have a central area to manage these styles. If we move them to multiple places, I don't see how it will be beneficial with the single usage of the component. It will only make the maintenance more difficult because we'll have multiple places to manage its styles. That's especially valid if we decide to move the interface structure around, which is definitely possible in the future.
That being said, I'm happy to alter this and move those particular styles to the parent if you feel strongly about it.
Contributor
There was a problem hiding this comment.
As I said, it's just a small thing. But now if I place it randomly in a page, it will take the full position of whatever is defined as "relative" while previously it was more predictible and just takes the full width and height of its direct container. Let's just say I was nitpicking here, it's not important.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
This PR fixes the site editor canvas edit mode button to occupy the entire frame canvas.
See #47676 for where this button was initially introduced.
I believe this was broken in #50222.
Why?
Right now, it's broken during the loading phase - the button appears below the canvas.
How?
We're just making the loading spinner absolute so the site editor canvas edit mode button will take the entire canvas space.
Testing Instructions
Testing Instructions for Keyboard
None
Screenshots or screencast
None