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.

Sign up for GitHub

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

Jump to bottom

RTL: Fix left/right alignments of blocks #3911

Merged
merged 3 commits into from
Jul 3, 2018
Merged

RTL: Fix left/right alignments of blocks #3911

merged 3 commits into from
Jul 3, 2018

Conversation

yoavf
Copy link
Contributor

@yoavf yoavf commented Dec 11, 2017 •
edited
Loading

Description

Prevent RTLCSS from reversing specific CSS rules related to left/right alignment of blocks
Similar to #3909, necessry after #3844

Without this change, left aligning a block puts on the right and vice versa.

Types of changes

CSS comments to prevent automatic flipping of CSS properties.

Checklist:

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

@yoavf yoavf changed the title Fix left/right alignments of blocks in RTL mode RTL: Fix left/right alignments of blocks Dec 11, 2017
youknowriad
youknowriad previously requested changes Dec 11, 2017
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. While in master the block alignments are reverted, they work correctly. In this PR the block alignment is broken (they show up out of the "centered" area)

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jan 27, 2018
@gziolo
Copy link
Member

gziolo commented Jan 27, 2018

@yoavf do you plan to finish this PR or should someone else take it over?

@yoavf
Copy link
Contributor Author

yoavf commented Jan 30, 2018 •
edited
Loading

@gziolo on it - updated the pr

@yoavf yoavf force-pushed the update/blocks-alignment-rtl branch from 886cda1 to 16e8846 Compare January 30, 2018 12:22
@gziolo gziolo requested a review from a team February 2, 2018 20:01
@gziolo gziolo removed the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 2, 2018
@tofumatt
Copy link
Member

Hey @yoavf, looks like there are a few conflicts here now but if you can update the branch and fix the conflicts I promise to review this ASAP (feel free to assign/ping me for review once it's updated) 😄

Sorry about not getting to this one 😞

@yoavf yoavf force-pushed the update/blocks-alignment-rtl branch 4 times, most recently from 9ed17ab to bec80de Compare June 21, 2018 10:27
@yoavf yoavf force-pushed the update/blocks-alignment-rtl branch from bec80de to 1e0bef3 Compare June 21, 2018 10:38
This was referenced Jun 21, 2018
Merged
Closed
@azaozz
Copy link
Contributor

azaozz commented Jun 21, 2018

Tested in few blocks (image, gallery, paragraph, list, table) and all works well.

@azaozz
Copy link
Contributor

azaozz commented Jun 25, 2018

In this PR the block alignment is broken (they show up out of the "centered" area)

This seems fixed with the latest changes here. Anything else blocking this?

tofumatt
tofumatt approved these changes Jun 26, 2018
View reviewed changes
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

All looks good and works for me.

A few minor nitpicks with the comments, but feel free to address them as you feel appropriate and then get this merged 😄 👍

editor/components/block-list/style.scss
@@ -366,7 +366,7 @@
}
}

// The padding collapses, but the outline is still 1px to compensate for.
// The padding collapses, but the outline is still 1px to compensate for.
Copy link
Member

Choose a reason for hiding this comment

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

To compensate for what? I think this comment is incomplete.

Copy link
Member

Choose a reason for hiding this comment

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

@tofumatt - can you update this comment yourself and merge this PR?

Copy link
Member

@tofumatt tofumatt Jul 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Totally, will do!

EDIT: I forgot about this PR and realised my issue is I have no idea what this means, but I'll figure it out and address...

editor/components/block-list/style.scss
@@ -375,19 +375,24 @@
// Left
&[data-align="left"] {
.editor-block-list__block-edit { // This is in the editor only, on the frontend, the img should be floated
Copy link
Member

Choose a reason for hiding this comment

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

I know it's not part of this PR but that's a weird indentation/comment placement, if anymore edits are made it'd be nice to tidy it up 😉

@tofumatt tofumatt dismissed youknowriad’s stale review June 26, 2018 19:27

Issues reported were fixed with a later commit.

@jasmussen jasmussen added this to the 3.2 milestone Jun 29, 2018
@tofumatt tofumatt mentioned this pull request Jul 3, 2018
Merged
@tofumatt tofumatt removed this from the 3.2 milestone Jul 3, 2018
@tofumatt tofumatt merged commit b332ebd into WordPress:master Jul 3, 2018
tofumatt added a commit that referenced this pull request Jul 3, 2018
@tofumatt
Revert "RTL: Fix left/right alignments of blocks (#3911)"
057f47b 
This reverts commit b332ebd.
@tofumatt tofumatt added this to the 3.2 milestone Jul 3, 2018
tofumatt added a commit that referenced this pull request Jul 3, 2018
@tofumatt
chore: Address comments in #3911 (#7690)
a713494 
* chore: Fix unclear comments
* chore: Tidy up inline comments
@yoavf yoavf deleted the update/blocks-alignment-rtl branch July 4, 2018 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tofumatt tofumatt tofumatt approved these changes

@gziolo gziolo gziolo left review comments

@youknowriad youknowriad youknowriad left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.2
Development

Successfully merging this pull request may close these issues.
6 participants
@yoavf @gziolo @tofumatt @azaozz @youknowriad @jasmussen