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

Updated theme.json schema descriptions to be more complete #48475

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

tomdevisser
Copy link
Member

Fixes #48450.
Props tresorama.

What?

Updated descriptions in theme.json's schema.

Why?

Some descriptions were very short and incomplete, which made them confusing.

How?

By taking suggestions on #48450 and adjusting theme how I saw fit.

Testing Instructions

  1. Create a theme.json
  2. Hover over the following
  • styles.typography
  • styles.color
  • styles.border
  • styles.filter
  • styles.shadow
  • styles.outline
  • settings.layout.contentSize
  1. Or check the commit

@tomdevisser tomdevisser added [Type] Enhancement A suggestion for improvement. Developer Experience Ideas about improving block and theme developer experience [Type] Developer Documentation Documentation for developers labels Feb 27, 2023
@tomdevisser tomdevisser self-assigned this Feb 27, 2023
@tomdevisser tomdevisser added the [Type] Copy Issues or PRs that need copy editing assistance label Feb 27, 2023
@t-hamano
Copy link
Contributor

@tomdevisser
Thank you for the PR!

This change might make sense, but unfortunately, these schemas are also used at the blocks and elements levels, so we can't include specific levels in the description.

blocks

elements

Personally, I feel that the description of the higher level properties will tell us to which level the change is being made. What do you think?

styles

elements_desc

@tomdevisser
Copy link
Member Author

@t-hamano Oh, sorry, hadn't noticed that. So there's no way to "decouple" these descriptions? There must be right?

@tomdevisser
Copy link
Member Author

Addition: In any case we could still update the description for settings.layout.contentSize, as that specifies it sets the max width which might be confusing, cause the width can still be bigger with different settings. I'm not saying my proposed option is the best option, but I do think it's an improvement.

@tresorama
Copy link

Addition: In any case we could still update the description for settings.layout.contentSize, as that specifies it sets the max width which might be confusing, cause the width can still be bigger with different settings. I'm not saying my proposed option is the best option, but I do think it's an improvement.

If this will be merged I suggest to also update the description for settings.layout.wideSize to be similar to settings.layout.contentSize, and to emphatize their relation to constrained layout.

@tomdevisser
Copy link
Member Author

emphatize their relation to constrained layout

@tresorama I deliberately didn't use the term constrained you used earlier, as it isn't being used anywhere. Not on the block settings and not anywhere else. It's not constrained, it's just the default container opposed to being extra wide. Not constrained opposed to default.

@tresorama
Copy link

@t-hamano Oh, sorry, hadn't noticed that. So there's no way to "decouple" these descriptions? There must be right?

If this is not possible maybe this can be somehow inspirational.

This is how Chakra let dev open documentation directly on context.
Schermata 2023-02-27 alle 19 12 30

Maybe put detailed description in doc site and link there from schema.
But i have no idea where is better to put these link in the schema because of "duplication".

I posted here because it's related.

@tresorama
Copy link

emphatize their relation to constrained layout

@tresorama I deliberately didn't use the term constrained you used earlier, as it isn't being used anywhere. Not on the block settings and not anywhere else. It's not constrained, it's just the default container opposed to being extra wide. Not constrained opposed to default.

Maybe i'm missing something, sorry but i started using Gutenberg in last 10 days .

In test that i have done I can only "see" that max-width (content or wide) is applied to a block only if that block is child of "Group" block (not Row or Stack).

If I go to site editor (not page editor) and place an "Image" at root level the UI doesn't let me choose between "wide (wideSize)" or "none (contentSize)", and in the frontend the"Image" is full width.
But if you nest the"Image" inside a "Group" you can now use "wide" or "none" width on the Image.

I noted that "Group" has a css class .is-layout-constrained that applies the max-width to its children.

This is why i was saying constrained, because of the css class.

Editor

Kapture.2023-02-27.at.20.26.03.mp4

Frontend
Schermata 2023-02-27 alle 20 33 14

@tomdevisser
Copy link
Member Author

tomdevisser commented Feb 27, 2023

This is why i was saying constrained, because of the css class.

@tresorama Got it, and makes sense, but from a frontend perspective when the alignment is "None", that's what you call "constrained". That felt weird to me, as "constrained" sounds like it's not the default option. But it also varies per block. I guess an Image is usually not a top-level block, but mainly used as an inner block to blocks like Group, Columns, Row, etc. That was my philosophy anyway, but you can look at it from both ways.

Let's see if anyone else has a strong opinion about this, I think it's fine either way but this one makes a bit more sense to me.

@tresorama
Copy link

...when the alignment is "None", that's what you call "constrained"

In my mental model :

  • flow layout , with is-layout-flow css class

    Is used by block placed at root level block in site editor (without parent block)
    Is like html default layout (elements with display block are stacked vertically, inline horizontally)
    It's what you call "Default"

  • constrained, with is-layout-constrained css class

    is used by Group block
    is like container-sm container-md of Bootstrap or Tailwind
    contentSize and wideSize are comparable to container-md and container-lg
    It requires a parent to be applied (the Group block)

  • flex, with is-layout-flex css class

    used by Stack and Row, maybe also Columns
    is like CSS flexbox

Let's see if anyone else has a strong opinion about this, I think it's fine either way but this one makes a bit more sense to me.

Of course this subject is "opinion" land. I have not spent enougt time with Gutenberg to have a strong opinion, let's wait for core maintainers.

I only add this:

  • theme.json is consumed by developer, so it should be easy for them, the final user of the website (content creattor) should not even know that a theme.json exists.
  • a developer sometimes perfers verbosity over ambiguity/confusion

@tomdevisser
Copy link
Member Author

@tresorama Yes, makes a lot of sense when looking at the classes, something I never need to cause I always use block variations, custom blocks (block.json) and theme.json for my styling where I use my own classes. As that's the recommended way (core classes could change at any time).

a developer sometimes prefers verbosity over ambiguity/confusion

Also agreed, let's see if others chime in with different opinions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience [Type] Copy Issues or PRs that need copy editing assistance [Type] Developer Documentation Documentation for developers [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: Improve theme.json ambiguos descriptions in the schema and in the doc site
3 participants