Extract the three color panels to their own global styles view #35400
Conversation
This file contains 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
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
mtias,
ciampo,
jasmussen and
jorgefilipecosta
youknowriad
commented
Oct 6, 2021
|
|
||
| export default function ColorPanel( { name } ) { | ||
| function ScreenBackgroundColor( { name } ) { |
There was a problem hiding this comment.
I have considered merging the three screens (background, text and link) into a single screen with a colorType prop to change its behavior but the code felt less readable and full with if/else. So I thought for now it's better to have separate screen (even with some code duplication).
youknowriad
commented
Oct 6, 2021
| import { getSupportedGlobalStylesPanels, useStyle } from './hooks'; | ||
| import Subtitle from './subtitle'; | ||
|
|
||
| function BackgroundColorItem( { name, parentMenu } ) { |
There was a problem hiding this comment.
I have considered merging the three items (background, text and link) into a single component with a colorType prop to change its behavior but the code felt less readable and full with if/else. So I thought for now it's better to have separate screen (even with some code duplication).
There was a problem hiding this comment.
This seems fine to me. We might want to create a "BlockElement" higher level component eventually, but we need to see how the typography ones evolve, and how we want to render this in the block inspector where it doesn't "navigate".
|
Size Change: +360 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
mtias
reviewed
Oct 6, 2021
packages/edit-site/src/components/global-styles/screen-colors.js
Outdated
Show resolved
Hide resolved
youknowriad
force-pushed
the
update/color-panel
branch
from
c7ac256
to
8b277a8
Compare
16 tasks
ciampo
reviewed
Oct 7, 2021
There was a problem hiding this comment.
Sorry if I'm a bit late to review this, I had a lot to catch up this week.
In general, it'd be great if we could slowly chip away at packages/edit-site/src/components/global-styles/style.scss by leveraging the components directly, until we don't need those styles at all.
Comment on lines
+37
to
+42
| .edit-site-global-styles-subtitle { | ||
| // Need to override the too specific styles for complementary areas. | ||
| margin-bottom: 0 !important; | ||
| text-transform: uppercase; | ||
| font-weight: 500; | ||
| } |
There was a problem hiding this comment.
Is this a sign that the Heading component is not fitting our design needs at the moment?
There was a problem hiding this comment.
potentially, I'm not sure if this uppercased heading is something that is already available and part of the "style guide"
This was referenced Oct 8, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Builds on top of #35264
Related to #34574
This PR extract the different color configurations into their own separate view and adds a group item to navigate to each one of them, like suggested in #34574
This PR doesn't update the design of the content of these views directly, they're still using the old color components for now.
Testing instructions