Skip to content

Edit Site: Fix site editor canvas edit mode button #53730

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.

Sign up for GitHub

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

Merged
tyxla merged 1 commit into from
Aug 16, 2023
Merged

Edit Site: Fix site editor canvas edit mode button #53730

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/edit-site/src/components/canvas-spinner/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
width: 100%;
height: 100%;
display: flex;
position: absolute;
top: 0;
left: 0;
Copy link
Contributor

@youknowriad youknowriad Aug 16, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@tyxla tyxla Aug 17, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@youknowriad youknowriad Aug 17, 2023

Choose a reason for hiding this comment

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

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.

opacity: 0;
align-items: center;
justify-content: center;
Expand Down