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

Move opinionated styles for Quote block #7449

Closed
wants to merge 1 commit into from
Closed

Move opinionated styles for Quote block #7449

wants to merge 1 commit into from

Conversation

BinaryMoon
Copy link

@BinaryMoon BinaryMoon commented Jun 21, 2018

Description

As per #7172 this moves opinionated styles for the Quote block to theme.scss

I actually moved all styles into theme.scss since all themes should have support for styling blockquotes correctly.

I've not submitted a pull request for Gutenberg before but if this goes ok then I'll likely go through all the blocks and update them appropriately.

How has this been tested?

I tested this locally in MAMP using my theme Carmack. This is the theme I run on my personal site. It's also available for sale on wordpress.com

The theme already has basic support for most of Gutenbergs features. To test it I added the line add_theme_support( 'wp-block-styles' ); to the after_setup_theme hook, and then tested the quote display before and after. This only affects the display of the blocks on the front end.

I also tested the theme on the editor by refreshing the editor and checking that the display did not change.

Types of changes

This is an improvement to the quote styles. It may alter themes that are relying on the current display of blockquotes. They can restore their display by adding add_theme_support( 'wp-block-styles' ); to the after_setup_theme hook

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Merge style.scss into theme.scss and remove style.scss

Most themes will already have styles for blockquotes so none of this is needed.
@BinaryMoon BinaryMoon changed the title Remove opinionated styles Remove opinionated styles for Quote block Jun 21, 2018
@BinaryMoon BinaryMoon changed the title Remove opinionated styles for Quote block Move opinionated styles for Quote block Jun 21, 2018
@jasmussen
Copy link
Contributor

Apologies for missing this pull request ❤️

I think that the opinionated styles were moved to theme.scss as part of a separate task to improve block variations, recently merged into 3.2. Can you take a look and see if this is the case?

I will also be looking at doing this for the remaining blocks.

Thank you for your PR.

@BinaryMoon
Copy link
Author

BinaryMoon commented Jun 29, 2018

Hi @jasmussen - @mtias suggested I do some pull requests to merge things here: #7172 (comment)

So that's what I started doing. There were some changes to quotes made in a previous ticket but they didn't have any effect.

I don't mind changing all the blocks, but I didn't want to make loads of changes until I knew I had the right process.

@jasmussen
Copy link
Contributor

For sure. I'm about to make a pull request that does something of the same. Mind if I CC you for a quick look?

@BinaryMoon
Copy link
Author

@jasmussen go for it

@jasmussen
Copy link
Contributor

I can confirm that in master, the blockquote styles are now opt-out-able, insofar as they now live in the theme.scss file. Sorry about the duplicate work, Ben, but thanks again for doing this. I think this PR can be closed.

I also created #7624 to start working on the remaining blocks. Please leave your thoughts there as to which block styles should be opt-out-able.

@jasmussen jasmussen closed this Jun 29, 2018
@BinaryMoon
Copy link
Author

BinaryMoon commented Jun 29, 2018

@jasmussen I think you're wrong here. There are still styles here:
https://github.com/WordPress/gutenberg/blob/master/core-blocks/quote/style.scss

These are styles I don't want in my theme.

The margins, padding, and line-height break my vertical rhythm. The font sizes break my font scale. There is an italic that is inconsistent with my blockquote styles.

Why do we need these things? My pull request fixes this.

@jasmussen
Copy link
Contributor

Oh sorry, I hope you didn't take offense at my closing the PR. Here it is reopened. I simply assumed it was fixed and did a little fast triaging.

@jasmussen jasmussen reopened this Jun 29, 2018
@BinaryMoon
Copy link
Author

Not offence, but I'm getting frustrated since I've been discussing this stuff in multiple issues on here and nobody seems to understand. I don't know if I'm not being clear or that people simply don't care.

There's more background in the first comment here: #7172

I think Gutenberg has a lot of potential but as someone who is trying to add support to my themes I am finding it rather challenging to override all of the styles without resorting to many resets & !importants which makes my themes code bigger and messier. I'm trying to work towards fixing this but it's proving difficult.

@jasmussen
Copy link
Contributor

I'm sorry you feel left out here, I'm particularly sorry no-one has come to review your PRs yet. In my case, I've simply been too buried in making the main editing canvas work and be theme style friendly for the past long time, and rushing to reach a feature complete stage. I'm finally getting time to focus on these things and your feedback will be immensely helpful in making sure we reach a solid balance here. I truly believe that we are just a few pull requests away from your concerns being addressed.

@aduth aduth added the Needs Design Feedback Needs general design feedback. label Sep 13, 2018
@jasmussen
Copy link
Contributor

This is how the quote blocks style.scss looks as of writing:

.wp-block-quote {
	&.is-style-large,
	&.is-large {
		margin: 0 0 16px;
		padding: 0 1em;

		p {
			font-size: 24px;
			font-style: italic;
			line-height: 1.6;
		}

		cite,
		footer {
			font-size: 18px;
			text-align: right;
		}
	}
}

And here's the "opinionated" theme.scss file:

.wp-block-quote {
	margin: 20px 0;

	cite,
	footer,
	&__citation {
		color: $dark-gray-300;
		font-size: $default-font-size;
		margin-top: 1em;
		position: relative;
		font-style: normal;
	}
}

.wp-block-quote:not(.is-large):not(.is-style-large) {
	border-left: 4px solid $black;
	padding-left: 1em;
}

The balance as it reads above is that variations applied to the quote are relatively generic but present for all themes (to ensure user expectations are met when a variation is applied), but the stock quote is unchanged for themes.

If we need to make the variation styles even more generic and agnostic, this can be done in new PRs, but for now I'm going to close this one as fixed, since the default quote is unstyled for themes unless they opt-in.

@jasmussen jasmussen closed this Oct 11, 2018
@jasmussen jasmussen removed the Needs Design Feedback Needs general design feedback. label Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants