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

Verse Block: Alignment uses inline styles #15751

Closed
CreativeDive opened this issue May 21, 2019 · 9 comments · Fixed by #16777
Closed

Verse Block: Alignment uses inline styles #15751

CreativeDive opened this issue May 21, 2019 · 9 comments · Fixed by #16777
Labels
[Block] Verse Affects the Verse block [Package] Block library /packages/block-library [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@CreativeDive
Copy link
Contributor

Currently the verse block get an inline style after selecting a specific alignment.

<pre style="text-align:left" class="wp-block-verse">Content</pre>

This is an unexpected behavior and should work like this:

<pre class="wp-block-verse alignleft">Content</pre>

Without adding the alignment class, I can't set any specific theme side styles for the alignment of verse block.

To style a poem block, the alignment class is necessary to get more design possibilities.

Is there a reason why you should not add the alignment class to this block type?

@swissspidy swissspidy added the [Block] Verse Affects the Verse block label May 23, 2019
@talldan talldan changed the title Verse Block: Alignment - unexpected behavior Verse Block: Alignment uses inline styles May 24, 2019
@talldan
Copy link
Contributor

talldan commented May 24, 2019

Hi @CreativeDive. Thanks for creating the issue. I hope you don't mind, I changed the title to be more specific.

It looks like text alignment works the same across other core blocks I've checked (paragraph, header, possibly others), they also use an inline style.

I had a look through some of the history in git, but couldn't find any justifications for an inline style over a class.

Pinging @jasmussen, who's very knowledgeable on these matters to see if he has any ideas.

@talldan talldan added [Type] Enhancement A suggestion for improvement. [Status] Needs More Info Follow-up required in order to be actionable. labels May 24, 2019
@jasmussen
Copy link
Contributor

Thanks for the ping. My knowledge extent here suggests that the person to ask is @ellatrix. She is the master of everything rich text. Ella any thoughts?

@codebymikey
Copy link

I have a similar issue with paragraphs and all other blocks which apply inline styles in their save function.

It makes everything a pain to theme, and the html starts to get really ugly/hacky quick (like a lot of page-builders out there, which is why I prefer Gutenberg).

The only things I believe require inline styles are custom attributes like backgrounds, margins and padding. Everything else imho should be class-based as a convention.

I'd recommend using a new class like aligntextleft rather than alignleft in order to avoid accidentally breaking existing themes which rely on floats for their alignleft type styles.

With the new styles being something along the lines of:

.aligntextleft { text-align: left }
.aligntextcenter { text-align: center }
.aligntextright { text-align: right }

This allows the theme designer to know when an alignment has been applied on the text and style it accordingly.

Hopefully someone who has more of a say on the architecture can provide more insight/help.

@gziolo gziolo added [Package] Block library /packages/block-library and removed [Status] Needs More Info Follow-up required in order to be actionable. labels Jun 10, 2019
@gziolo
Copy link
Member

gziolo commented Jun 10, 2019

I'm proposing a fix which could get applied to all blocks which use text alignment in #16035.

@codebymikey
Copy link

codebymikey commented Jun 10, 2019

That's really amazing news @gziolo, and the implementation looks very clean.

Hopefully the PR lands soon, and is used as a convention going forward. And we can do away with non-essential inline attributes on all blocks.

@gziolo
Copy link
Member

gziolo commented Jun 14, 2019

Now that #16035 has landed for Heading block, we should replicate it for Verse as well.

@ellatrix
Copy link
Member

My knowledge extent here suggests that the person to ask is @ellatrix. She is the master of everything rich text. Ella any thoughts?

Sounds like this is not rich text related. Alignment is a block level attribute. Looks like @gziolo is taking care of this now.

@gziolo
Copy link
Member

gziolo commented Jul 26, 2019

I opened #16777 which will resolve this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Verse Affects the Verse block [Package] Block library /packages/block-library [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants