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

Reviewing Global Styles main panel #38934

Closed
6 of 7 tasks
pablohoneyhoney opened this issue Feb 20, 2022 · 11 comments
Closed
6 of 7 tasks

Reviewing Global Styles main panel #38934

pablohoneyhoney opened this issue Feb 20, 2022 · 11 comments
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.

Comments

@pablohoneyhoney
Copy link

pablohoneyhoney commented Feb 20, 2022

With GS live, it's pertinent to take a look at how elements are presented in the interface compared to the original intent. There are likely valid reasons (technical, functional, etc.) for certain objects to evolve and be at the state they are, while some do need further advancement to depict the best experience and UI possible.

Generally, it's in a fantastic state and it's already opening up new avenues of thinking. It is worth however to look at the foundational parts to be able to grow healthier. The spacings, alignments, and scales are the main aspects that could use some help.

The intent of this issue is to itemize and discuss certain improvements in the root panel, and it will be useful to later review other areas of the full styles experience. It is not the intent to look at larger functional changes like styles switching or how colors get represented in the preview from the theme –that's why I chose to use a single color to cover the below.

Current vs. Original Design

Screen Shot 2022-02-20 at 1 23 26 PM

  • Adjust styles preview box size. Global Styles Sidebar: tweak preview box #39978
    Current box is 248x150 while the original was 248x152, which matches more precisely a grid of 8 and provides enough white space for typographies of different structure.

Screen Shot 2022-02-20 at 12 22 24 PM

  • Adjust color swatches size. Global Styles Sidebar: tweak preview box #39978
    Current swatch is 40x40 while the original was 32x32, which creates a more balanced presence with the typography as they deserve equal representation as a style preview. The current example (left) is quite bulky.

Screen Shot 2022-02-20 at 12 33 31 PM

Screen Shot 2022-02-20 at 12 46 06 PM

Worth reviewing them all but the elements seem to have a transparency or a slight grey. If I'm not mistaken the color code used is: rgba(0, 0, 0, 0.847). The original idea was to leverage the G2 dark grey rgba(30, 30, 30) or #1E1E1E as the principle dark color, which might be a different topic all together. But at least labels are better off without that grey or transparent look so they align with the rest of elements in the interface, like the icons.

Screen Shot 2022-02-20 at 12 58 46 PM

There's a slight horizontal unbalance between the icon and label that is worth looking at, as it might be an issue with the icon set up or the label position. That is also applicable to the relationship between block and arrow.

Screen Shot 2022-02-20 at 1 00 37 PM

  • Consider removing the heading separator.

This might be a topic to look at as we look at the whole flow, since this separator is likely inherited from the main component and it's also perhaps needed in interior sections. Anyhow, the separator isn't needed in the main panel when it lives with the style preview. This can also bring up the preview a bit higher in the panel.

Screen Shot 2022-02-20 at 1 07 28 PM

It also causes however some frictions currently when the element represents its own separator (as below).

Screen Shot 2022-02-20 at 1 02 54 PM

While there's no need to match the original spacing, the current has all elements fairly expanded and could use some tighting up for better visual grouping and generally reduce the height of the panel elements. If it helps for reference, the original intent had a padding of 16, but the one live. While correct, it also includes the separator component own padding, which increases significantly the spacing.

Screen Shot 2022-02-20 at 1 21 48 PM

The spacing discordance also applies to styles elements.

Screen Shot 2022-02-20 at 1 30 54 PM

@pablohoneyhoney pablohoneyhoney added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Feb 20, 2022
@annezazu annezazu added the [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. label Feb 21, 2022
@ciampo
Copy link
Contributor

ciampo commented Apr 1, 2022

Hey @pablohoneyhoney, here are my considerations (and a few questions) after a brief investigation:

Adjust styles preview box size

This can be improved as you suggest with quick adjustment directly to the Global Styles preview in the sidebar.

Adjust color swatches size

This is also a quick adjustment that can be made similarly to the previous one.

When I look at what we currently have on trunk, the size of the color swatch appears to be 30 (and not 40). There may have been recent changes, compared to when the screenshots above were taken — this is what I see as of today when loading the latest version of the Global Styles sidebar on my local machine:

Screenshot 2022-04-01 at 12 40 00

I'm happy to update the size of the color swatch to 32, but I wonder if the rest of the spacing (and the font size) would also need updating? It would be useful to have a complete design spec for the Styles Preview card

Smooth the corners of the styles box

Wrapping the <StylesPreview /> component inside <CardMedia /> should definitely improve the situation (🤞 ), although there's always the chance that browsers rendering engines can introduce some sub-pixel / anti-aliasing artefacts.

Correct labels colors

Worth reviewing them all but the elements seem to have a transparency or a slight grey. If I'm not mistaken the color code used is: rgba(0, 0, 0, 0.847).

I wasn't able to find elements with such transparent color, these elements seem to be black (#000000) for me.

The original idea was to leverage the G2 dark grey rgba(30, 30, 30) or #1E1E1E as the principle dark color

I can see the #1e1e1e color being applied to all contents of "complementary area" (a part of the codebase that I never worked on before). But currently, there are a few style rules with higher specificity that are basically applying the black color to all content — one of them is this style rule from the Surface component.

I'd be reluctant to make changes directly in the source components — instead, we can look into applying style overrides specifically to the global styles sidebar in order to change the text color.

Align icons with labels

This is an adjustment that can be easily applied as a "global" adjustment to all icons — it would be more complicated if we would need to tweak the alignment on an icon-by-icon basis.

Consider removing the heading separator

As you also mentioned, the header used in the Global Styles sidebar is a shared component called ComplementaryAreaHeader that lives in a separate package (@wordpress/interface).

The ComplementaryAreaHeader component gets most of its styles (including the bottom border) by using the components-panel__header classname (instead of using the actual PanelHeader component from @wordpress/components — this by itself is a dangerous anti-pattern!).

In short, we can't remove the border styles from the "source" (i.e. the styles of the PanelHeader component).

I see two choices — both feel slightly hacky, and I don't personally know the @wordpress/interface package well enough to be certain about what's the best one:

  • manually override the styles of the header (and remove the border) only in the Global Styles sidebar
  • add a prop to ComplementaryArea to be able to disable the header's bottom border

Maybe @jorgefilipecosta can help here?

Review spacings between all elements

The current spacing is a result of different paddings (mostly Card and ItemGroup) and a margin of 1.5em on the separator (which again, comes from the styles applied by the "complementary area").

Adjustments can be definitely made. It would be interesting to see which of these adjustments can happen at the "source" and which need to be applied as a style override specifically for Global Styles


I'll follow up shortly with a few PRs tackling the points highlighted above.

@annezazu
Copy link
Contributor

annezazu commented Apr 6, 2022

During the 6.0 walkthrough, the prominence of switching styles was raised and I wanted to flag here since this seems to capture an overview of current refinements to the Global Styles panel:

Screen Shot 2022-04-05 at 6 40 32 PM

Currently, the "Browse styles" matches the design of the "Blocks" section below it, when perhaps it should be fairly distinct. I'll leave it to you all to figure out how best to proceed but wanted to close the loop here as it seems that the above designs existed before style variations were being worked on. cc @critterverse who has been working on the animation and previews. If you'd prefer this be an additional issue, I'm happy to open one.

Of note, you can use this gist (and the instructions in the comment) to recreate having style options locally.

@ciampo
Copy link
Contributor

ciampo commented Apr 6, 2022

I'll let @pablohoneyhoney and @jasmussen digest the feedback here, including whether this should be handed in a separate issue

@pablohoneyhoney
Copy link
Author

Apologies @ciampo mind clarifying what you mean with this – I'm not sure I understand or link it to your notes above:

including whether this should be handed in a separate issue

Regarding the rest, all of those suggestions and tweaks are already good progress and worth doing as a next iteration. We can keep this issue open for further improvement notes, or create a separate one once all is addressed or nuanced.

Currently, the "Browse styles" matches the design of the "Blocks" section below it, when perhaps it should be fairly distinct.

I agree with this @annezazu and believe I mentioned it in the context of @critterverse iterations in that area.

@ciampo
Copy link
Contributor

ciampo commented Apr 20, 2022

mind clarifying what you mean with this – I'm not sure I understand or link it to your notes above:

including whether this should be handed in a separate issue

Sure! My reasoning here is that @annezazu's feedback is still quite at a high-level (differentiate the design of the "Browse styles" and the "Blocks" sections), and so I proposed that we may move it to a separate GitHub issue, where folks can discuss and work on a design spec until it's ready to be executed and tracked separately from the initial list of tweaks already suggested in this issue.

Relatively to this issue, @mirka and I have been progressively addressing most of the feedback. The only areas that we haven't tackled yet are:

  • "Consider removing the heading separator" (this is a non-trivial fix — I left some more details in this comment, including a ping to @jorgefilipecosta in case he had any suggestions)
  • "Review spacings between all elements" (not trivial, since we'll need to balance changes to the core components and tweaks specific to what's called the "complementary area" and the sidebar)

@jasmussen
Copy link
Contributor

Agreed there are lots of small fixes we can make that will benefit the UI as it exists today and for the foreseeable future.

One additional consideration is how global styles can evolve in the future. For example, #36667#issuecomment-1087711883 suggests moving it to the left on a dark background and shown in context of other site-wide tools such as "Navigation". That would absorb the "Templates" item as well. Would that make room for "Browse styles" to live as a normal menu item next to Color and Typography, perhaps even Blocks? Or do they still need sectioning off and/or live in the context of the style card?

@critterverse
Copy link
Contributor

Shared a few ideas for iterating on the Browse styles button in #40478 :)

@jasmussen
Copy link
Contributor

One quick note worth considering. The borderless heading as designed looks great and makes the whole panel feel a bit more like a cohesive unit.

Actually removing that border might require a little extra effort, however. On very small viewports, or as soon as you click into a sub-section, the header stays fixed at the top and a scrollbar can appear — in those cases keeping the border might be useful:
Screenshot 2022-04-22 at 08 18 32

The borderless look works okay with scrollbars in the following experiment, but it might work better if the border would appear contextually with a scrollbar:

experiment

Given that #36667 explores global styles on a dark background, it may not be worth exploring such a border-behavior right at this point.

@ciampo
Copy link
Contributor

ciampo commented Apr 22, 2022

it may not be worth exploring such a border-behavior right at this point.

I agree — removing the border from the heading feels still very much an idea that needs more exploration before it can be implemented (plus, it wouldn't be a trivial change, as explained before).

I am in favour of delaying the work on this potential change until it feels ready for implementation.

@jorgefilipecosta
Copy link
Member

Hi @ciampo,

"Consider removing the heading separator" (this is a non-trivial fix — I left some more details in #38934 (comment), including a ping to @jorgefilipecosta in case he had any suggestions)

For this task, I think the solution "manually override the styles of the header (and remove the border) only in the Global Styles sidebar" is the right one. We have a class for the global styles sidebar we can use specific styles to customize that sidebar specifically because it has very specific needs.

@ciampo
Copy link
Contributor

ciampo commented Apr 28, 2022

Update:

From the original issue description, @mirka and I tackled all points except the one about removing the heading separator. I kept the original description up to date with references to all of the PRs that we merged for each point.

Regarding the heading separator, I agree with @jasmussen 's message — in short:

it may not be worth exploring such a border-behavior right at this point.

This behaviour should be implemented (ideally by someone more knowledgeable about this part of the codebase than myself) only once it's in a more polished state design-wise.

For this task, I think the solution "manually override the styles of the header (and remove the border) only in the Global Styles sidebar" is the right one. We have a class for the global styles sidebar we can use specific styles to customize that sidebar specifically because it has very specific needs.

I agree that, in case we wanted to implement this change, manually overriding the header styles only in the Global Styles sidebar would be the quickest way. At the same time, it feels like we should still try to tidy these styles up more at the root (i.e. ComplementaryArea), rather than adding one more layer of style overrides.

In the light of the above, I believe that we can close this issue:


Side note: in the upcoming weeks I'm going to try to partially rewrite ComplementaryAreaHeader so that it doesn't use hardcoded classnames from the Panel component — these changes won't really be related to what is being discussed in this PR, it's mostly a refactor to avoid this anti-pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.
Projects
None yet
Development

No branches or pull requests

6 participants