Fix default text alignment

[ellatrix]

This PR makes the left text align button active when there is no text alignment set, and also removes the attribute when the alignment is set to left.

[dmsnell]

It would be nice to break this into two PRs: one that modifies the behavior of the left alignment; one that changes what it means to save attributes.

To this point I haven't heard discussion about whether we should save null values and while I could see value in trimming them out, I think it's worth making sure we don't want to save them.

In any case, was there a reason to not only change the condition but also change the code structure?

[ellatrix]

You mean switching away from ! ( ... ) ? ... : ...? I found it easier to read, sorry. :)

[ellatrix]

Regarding striping out null, I find it a bit strange to define as undefined and it doesn't seem to play well with React. Also, they strip out null as well.

[ellatrix]

E.g. style={ { textAlign: align } } when it updates from a string to undefined, it won't update at all. When it changes to null, it does.

[dmsnell]

I found it easier to read, sorry.

let's leave personal styling to separate PRs? I mean, it's fine, but I find the ternary easier to read in that it indicates that the return value is dependent on the condition ¯\_(ツ)_/¯

[dmsnell]

I find it a bit strange to define as undefined and it doesn't seem to play well with React. Also, they strip out null as well.

what do you mean here? React doesn't touch null values passed through as props.

[ellatrix]

Sorry for commenting in multiple comments: #1496 (comment)

[dmsnell]

Sorry for commenting in multiple comments: #1496 (comment)

?? I don't know what you are apologizing for but I was not offended 😄

[ellatrix]

For the confusing multiple comments that might pop up with a delay. :)

[ellatrix]

Okay, let's keep undefined. :)

[jasmussen]

This PR makes the left text align button active when there is no text alignment set, and also removes the attribute when the alignment is set to left.

Interestingly, @youknowriad actually made a PR just like this way back in the day, because I had the same instinct as you had. Then I realized that both the old WordPress editor, as well as Google Docs and many others behaved this way: by default, text has no alignment. And even if that "no alignment" happens to be the same as left aligned visually, selecting that text and pressing "left align" applies an explicit left alignment where none was before.

As a result, I'm not actually completely sure what we should do here, and I could go either way.

On one hand, the behavior currently in master matches most other text editors.

On the other hand, we are recasting paragraphs as blocks, and the alignments to be "global" to that block, and so perhaps this begs a rethinking of the behavior.

Any thoughts, @melchoyce @folletto?

[folletto]

I did a quick benchmark.

Google Docs does it on a brand new document (even if it's then possible to deselect it and nothing changes):

Pages does it:

Word does it (old version I have):

—

My take is that it was a technicality as you touched too: if it was selected it explicitly added a text-align rule to override anything the stylesheet would have done later in the theme, while if not selected it just inherited. This avoid the scenario where we assume "it's left aligned" and then the theme justifies it (or centers it).

So if I currently explicitly left-align in WP Admin:

And if I don't:

It's yet again one of the quirks due to the high flexibility of WordPress editor and themes, and inheriting from the complexity of CSS.

My take would be to try if possible without too much overriding going on to be explicit and do left-align by default (toggled), regardless of what the theme will do later.

I can see three options:

1. We allow to "deselect" an alignment (and default there).
2. We default to left-align — in practice removing explicit "left-align" inline CSS style.
3. We default to left-align, but we allow a hidden state triggered when the alignment is clicked twice (the icon might change in a left align with a lock, or something).

For ease of release, I'd probably go by keeping the current behaviour (unselected = no explicit style, assumes theme is left-aligning / inheritance), and explore alternatives after Gutenberg ships.

[ellatrix]

For ease of release, I'd probably go by keeping the current behaviour (unselected = no explicit style, assumes theme is left-aligning / inheritance), and explore alternatives after Gutenberg ships.

I agree. :)