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

Rename default block styles to "Default" #13670

Merged
merged 2 commits into from
Feb 14, 2019

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Feb 5, 2019

Resolves #13660.

This aligns name we're using in the UI with the classname, and also lets us be a little less prescriptive about what exactly the default block styles per theme should look like.

  • Separator block: Short Line ➡️ Default
  • Blockquote block: Regular ➡️ Default
  • Pullquote block: Regular ➡️ Default
  • Table block: Regular ➡️ Default
  • Button block: Rounded ➡️ Default

Resolves #13660.

This aligns the classname with the actual name we're using in the UI, and also  allows for a bit more flexibility for default block styles per theme.
@kjellr kjellr added [Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. labels Feb 5, 2019
@kjellr kjellr self-assigned this Feb 5, 2019
@kjellr kjellr requested review from mapk and jasmussen February 5, 2019 13:49
@jasmussen
Copy link
Contributor

Love it, thank you! 🤟 from me, I guess it just needs a code sanity check. Pretty sure this doesn't require deprecation handlers.

@@ -78,7 +78,7 @@ export const settings = {
},

styles: [
{ name: 'default', label: _x( 'Rounded', 'block style' ), isDefault: true },
{ name: 'default', label: _x( 'Default', 'block style' ), isDefault: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we landed a PR recently where we could drop these entirely from here and they will be added automatically (needs verification)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get a reference to that PR here? What exactly can we drop?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure sorry it wasn't that clear.

Here's the PR #12519

I think the entire line can be dropped (the default style)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested this, and it almost works. The block has the correct styles by default, but the "Default" style option does not appear in the style picker:

screen shot 2019-02-05 at 12 51 47 pm

Since it's not there, if you select an alternate style, there'd be no way to switch back to the default. 😕

So for now at least, I think we do need these lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm weird, I though it was the intent of the linked PR. pinging @swissspidy to check my assumptions :) but feel free to move forward with the PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that was the goal of #12519 🤔 No idea why it doesn't work, I would have to investigate.

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 5, 2019
@mapk mapk removed the Needs Design Feedback Needs general design feedback. label Feb 5, 2019
@ZebulanStanphill
Copy link
Member

Since the "Rounded" style for buttons is now just "Default", wouldn't it make sense to also add a dedicated "Rounded" style that themes can choose to support if their default isn't rounded? Same for other blocks where the default style wasn't previously just called "Regular".

@youknowriad
Copy link
Contributor

@ZebulanStanphill nothing stops themes from registering custom style variations.

@ZebulanStanphill
Copy link
Member

@youknowriad I know, but I thought it was kind of weird to provide a core style for a long line separator but not a dedicated core style for a short line separator.

Copy link
Contributor

@mapk mapk left a comment

Choose a reason for hiding this comment

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

Just tested and everything appears to be working great! I've a few notes.

  1. The is-style-default class didn't appear when I added the block. It only appeared when I selected it from the block style variations. Maybe this is how it always worked?
  2. The Table block show default in the Block Styles, but uses the class is-style-regular in the HTML. Is this right?

@kjellr
Copy link
Contributor Author

kjellr commented Feb 11, 2019

  1. The is-style-default class didn't appear when I added the block. It only appeared when I selected it from the block style variations. Maybe this is how it always worked?

Yeah, that's how it behaves today.

  1. The Table block show default in the Block Styles, but uses the class is-style-regular in the HTML. Is this right?

Hmm... you're right. That's not ideal. Ideally we should be using is-style-default there. But changing that up will be a breaking change for some themes, so I'm not sure the best way to handle that. My inclination is to leave it as is, but I could use another viewpoint on that.

@mapk
Copy link
Contributor

mapk commented Feb 11, 2019

Thanks for addressing those points, @kjellr.
I don't think number 2 should hold this PR up. LGTM

@mapk mapk self-requested a review February 14, 2019 16:45
Copy link
Contributor

@mapk mapk left a comment

Choose a reason for hiding this comment

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

Alrighty, :shipit:

@kjellr
Copy link
Contributor Author

kjellr commented Feb 14, 2019

Thanks, everyone!

@kjellr kjellr merged commit baf733c into master Feb 14, 2019
@kjellr kjellr deleted the update/default-block-style-names branch February 14, 2019 17:10
@webmandesign
Copy link
Contributor

Hi guys,

I'm really too late to the party here, sorry, but still I can see an issue with Separator block styles (and probably other blocks too):

styles: [
	{ name: 'default', label: __( 'Default' ), isDefault: true },
	{ name: 'wide', label: __( 'Wide Line' ) },
	{ name: 'dots', label: __( 'Dots' ) },
],

If a theme styles <hr> wide by default (which I'm sure many themes do), than a user is offered 2 same options here: "Defalut" and "Wide line".

Wouldn't this cause UX issues?
How could a theme user select a "Short line" style then?
Or "Short line" style is not available any more now?

@swissspidy
Copy link
Member

@webmandesign See #14102.

@webmandesign
Copy link
Contributor

Thanks @swissspidy

youknowriad pushed a commit that referenced this pull request Mar 6, 2019
Resolves #13660.

This aligns the classname with the actual name we're using in the UI, and also  allows for a bit more flexibility for default block styles per theme.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
Resolves #13660.

This aligns the classname with the actual name we're using in the UI, and also  allows for a bit more flexibility for default block styles per theme.
This was referenced Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants