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

UI: Polish canvas and sidebar for 7.0 #18894

Merged
merged 22 commits into from
Aug 18, 2022
Merged

UI: Polish canvas and sidebar for 7.0 #18894

merged 22 commits into from
Aug 18, 2022

Conversation

MichaelArestad
Copy link
Contributor

What I did

I removed the margins around the preview giving folks back some pixels. This required decoupling the theme's page margin from the draggers, which had some wild side effects before.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

@@ -2,13 +2,15 @@
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="theme-color" content="<%= themeColor %>" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmeasday Do you have any ideas on how to get the theme color to show here? If not, I can pull this line of code.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, not sure, I think @ndelangen knows the most about themes

Copy link
Member

Choose a reason for hiding this comment

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

No way to do it there, as the theme is determined at runtime.
And in fact can change at runtime.

At least that's how it is at this point, and will likely stay in 7.0 because changing that will be a breaking change.

Currently users define their theme in manager.ts but in order to know it at build time, it'd have to be moved to main.ts or some preset value.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the heads up Norbert! @MichaelArestad let's punt on this meta tag

@domyen domyen changed the title Remove margins around the canvas Polish Canvas and Sidebar UI for 7.0 Aug 13, 2022
@MichaelArestad
Copy link
Contributor Author

Rebased with the new icons.

@shilman shilman marked this pull request as ready for review August 18, 2022 11:14
@shilman shilman changed the title Polish Canvas and Sidebar UI for 7.0 UI: Polish canvas and sidebar for 7.0 Aug 18, 2022
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @yannbf for the story fixes!

@shilman shilman merged commit b097168 into next Aug 18, 2022
@shilman shilman deleted the remove-canvas-margin branch August 18, 2022 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants