Address "wide" alignment programatically #812

mtias · 2017-05-16T18:51:11Z

This currently showcases the desired behaviour we want.

What I like is that it restricts side-effects that overflows and vw seem to have. What I dislike is that we need to dispatch when changes that affect content width happen (collapsing wp-admin sidebar manually).

aduth

A related need for editor bounds (top, right, left) has been raised at https://github.com/WordPress/gutenberg/pull/839/files#diff-77cea533850e86cf5a0031b0acc4c0b2R40

aduth · 2017-05-19T15:54:06Z

editor/state.js

@@ -303,6 +303,21 @@ export function isSidebarOpened( state = false, action ) {
 	return state;
 }

+export function editorWidth( state = null, action ) {
+	const layout = document.querySelector( '.editor-layout__content' );


These seem like arguments that should be passed in through action properties, not extracted by the reducer itself.

Given the same arguments, it should calculate the next state and return it. No surprises. No side effects. No API calls. No mutations. Just a calculation.

http://redux.js.org/docs/basics/Reducers.html#handling-actions

Also, every reducer is called for every action, so these query selectors are executed very often, and almost always needlessly so. At the very least we should check action.type before performing any queries.

@aduth passing as action properties would mean we need to calculate wherever we dispatch the action, which doesn't seem great.

which doesn't seem great.

In what way? We're not really computing much. We could persist references to the DOM nodes. Apparently offsetWidth can force a reflow, but if it's tied to resize event, shouldn't be a concern (or, rather, we're going to have to call it anyways).

I mean more about cases where we may dispatch this from other places of the app, like the sidebar toggle. We shouldn't have to repeat the code there, I think.

Hmm, might be middleware territory in an ideal solution then.

aduth · 2017-05-23T12:14:36Z

editor/layout/index.js

+class Layout extends wp.element.Component {
+
+	componentDidMount() {
+		window.addEventListener( 'resize', this.props.setEditorWidth );


We might consider debouncing this very noisy event.

We might consider debouncing this very noisy event.

This ^

aduth · 2017-05-23T12:15:26Z

editor/layout/index.js

+	}
+
+	componentWillUnmount() {
+		window.removeEventListener( 'resize', this.props.setEditorWidth );


We should confirm that mapDispatchToProps is only called once for the lifetime of the component. Otherwise we run the risk of the reference here to setEditorWidth not being the same as the one assigned in componentDidMount, leaving zombie event listeners.

aduth · 2017-06-02T18:43:39Z

blocks/library/image/style.scss

@@ -17,9 +17,14 @@
 	}

 	&[data-align="wide"] {
+		margin-left: -24%;


How well does this work on smaller viewports?

Hmm yes this, for some reason, doesn't actually scale to the viewport. Replacing it with 5vw is seems to behave as intended.

jasmussen · 2017-06-05T07:40:40Z

@mtias I pushed a few responsive fixes to this branch, which will also benefit master. Do you think we can get this branch shipped to master this week? It'd be real nice to have.

mtias · 2017-06-05T13:17:01Z

Yes, we'll get something this week. Discussing with @aduth the tradeoffs of the implementation.

aduth · 2017-06-05T20:06:09Z

Somewhat related: #1023

This also adds a few improvements from the now deleted branch, #799, notably: - clearing for video blocks. The responsive trickery means we have to, or we will get odd behavior otherwise - better collapsing margins fix for the toolbar.

Update when sidebar is toggled.

There were some issues with centering blocks, due to the fact that it has a block mover on the left. This commit simplifies that math, so the block mover doesn't actually affect the padding of the block, but rather uses negative space, making the math simpler. Additionally, it adds some responsive fixes for the full wide toolbar positioning.

aduth

Much happier to be solving this with CSS.

aduth · 2017-06-07T18:59:33Z

blocks/library/image/style.scss

 		padding-left: 0;
 		padding-right: 0;
-		margin-right: -#{ $block-padding + $block-mover-margin };	/* Compensate for .editor-visual-editor centering padding */
+		box-sizing: border-box;


Shouldn't need this:

https://github.com/WordPress/gutenberg/blob/73bd4ab/editor/assets/stylesheets/main.scss#L17-L20

aduth · 2017-06-07T18:59:58Z

blocks/library/image/style.scss

-			margin-left: auto;
-			margin-right: auto;
-		}
+			max-width: $visual-editor-max-width - ( ( $block-padding + $block-padding + $block-mover-margin ) * 2 );;


Double semi-colon ending.

Should we move following comments before the max-width line?

👍

I think the max-width was removed just now.

aduth · 2017-06-07T19:03:50Z

blocks/library/image/style.scss

+			margin-right: $block-mover-padding-hidden + $block-padding;
+
+			@include break-small() {
+				width: 100%;


The position of the controls isn't quite right when at wide or full alignments. You can see it move left and right when toggling between an image "Align left" and "Align wide".

I think we need to assign width: $visual-editor-max-width; max-width: none;

We are emulating floats using margins. This works well enough, but has some responsive implications. This commit addresses those, by creating a mix-in that simplifies the breakpoints. I couldn't quite figure out where to best place this mixin, and we also probably want to extract both the mixin, the variables, and even the left/right alignment code so that other blocks that utilize these don't have to copy the whole bit of code. But I wanted to push this so we can test, and then discuss.

Clean up double semicolons, better center toolbars, remove border-box.

jasmussen · 2017-06-07T19:31:47Z

Addressed the feedback items, and also made the floats work with responsive.

Take an extra look at a4d712e — I wrote a small mix-in to simplify the responsive stuff a bit. Thankfully it's enough with three rules.

Essentially we have to deal with responsive breakpoints that are unique to the editor, and unique to whether post settings is open or closed.

I could use advice on:

how the mixin can be improved
where it should live
per item 2, where it should live so that we can re-use the same code for any block that lets you left or right float an item.

I'm antsy to get this branch in though, so perhaps it'd be good if we could separate feedback in two groups. 1 — improving the code so we can merge this, and 2 — feedback we can address in a later PR. Thanks.

aduth · 2017-06-07T20:41:03Z

where it should live

We could move it to apply to any .editor-visual-editor__block. Is sorta how we're already treating it anyways with getEditWrapperProps.

jasmussen · 2017-06-08T08:40:01Z

Okay, took your advice and moved both the mixin and the alignments.

@mtias what do you think? Ready to go?

jasmussen · 2017-06-08T08:46:22Z

This branch, when merged, fixes #784 and #546.

mtias · 2017-06-08T09:20:31Z

This is looking good. One side effect is we are losing the click-on-canvas-to-deselect.

This commit hides the movers on floated images. We can look at reintroducing them for floats and full width images in a separate PR. It also fixes another responsive issue where the sidebar is stickied.

mtias added [Feature] Blocks Overall functionality of blocks General Interface Parts of the UI which don't fall neatly under other labels. [Status] In Progress Tracking issues with work in progress labels May 16, 2017

mtias force-pushed the fix/full-width branch from 5978dc8 to 8dc24a9 Compare May 17, 2017 12:07

jasmussen mentioned this pull request May 19, 2017

Add alignment buttons for embed #840

Merged

aduth reviewed May 19, 2017

View reviewed changes

jasmussen added this to the Beta milestone May 22, 2017

jasmussen added the [Priority] High Used to indicate top priority items that need quick attention label May 22, 2017

ellatrix assigned mtias May 22, 2017

mtias force-pushed the fix/full-width branch from 8dc24a9 to 30e7902 Compare May 23, 2017 10:25

aduth reviewed May 23, 2017

View reviewed changes

mtias force-pushed the fix/full-width branch from c1b0df5 to 3584795 Compare May 23, 2017 15:47

jasmussen modified the milestones: May Week 4: Beta, May 29 to Jun 2 May 30, 2017

mtias force-pushed the fix/full-width branch 2 times, most recently from 65625f9 to 390e1a5 Compare June 2, 2017 16:10

aduth reviewed Jun 2, 2017

View reviewed changes

jasmussen modified the milestones: May 29 to Jun 2, Beta Jun 5, 2017

jasmussen mentioned this pull request Jun 6, 2017

Polish responsive a bit #1035

Merged

mtias removed this from the Beta milestone Jun 6, 2017

mtias force-pushed the fix/full-width branch 2 times, most recently from 61bf132 to 79fe513 Compare June 7, 2017 15:44

jasmussen and others added 10 commits June 7, 2017 18:15

Remove full-width trick pending better one

549228d

This also adds a few improvements from the now deleted branch, #799, notably: - clearing for video blocks. The responsive trickery means we have to, or we will get odd behavior otherwise - better collapsing margins fix for the toolbar.

Calculate width and positioning for align:wide attr.

9968972

Polish the toolbar centering math.

4d68c23

Keep track of editor width values in redux state.

e641ccb

Update when sidebar is toggled.

Fix issue with full width editor toolbar.

274b0fa

Add wide and full-width options to images.

7325ba3

Position toolbar in wide image option.

662aab6

Completely different approach using CSS grid.

aaeb881

Rework it all again with percentages.

b7888a8

mtias force-pushed the fix/full-width branch from 096cc4c to b7888a8 Compare June 7, 2017 16:18

mtias removed the [Status] In Progress Tracking issues with work in progress label Jun 7, 2017

More elegant calculation of margins.

5e999c9

aduth reviewed Jun 7, 2017

View reviewed changes

jasmussen added 2 commits June 7, 2017 21:19

Address feedback.

3c883b7

Clean up double semicolons, better center toolbars, remove border-box.

Move mixin, genericize alignments.

9ea3743

Hide movers and fix sticky menu

1923ade

This commit hides the movers on floated images. We can look at reintroducing them for floats and full width images in a separate PR. It also fixes another responsive issue where the sidebar is stickied.

mtias merged commit b84397f into master Jun 8, 2017

mtias deleted the fix/full-width branch June 8, 2017 09:37

youknowriad mentioned this pull request Jun 8, 2017

Blocks: Fix clearing selection on canvas click #1079

Merged

monzerfoda mentioned this pull request Feb 2, 2024

[Snyk] Security upgrade babel-eslint from 8.0.3 to 10.1.0 monzerfoda/gutenberg#21

Open

makbarGroup mentioned this pull request Feb 3, 2024

[Snyk] Security upgrade babel-eslint from 8.0.3 to 10.1.0 makbarGroup/gutenberg#57

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address "wide" alignment programatically #812

Address "wide" alignment programatically #812

mtias commented May 16, 2017 •

edited

Loading

aduth left a comment

aduth May 19, 2017

aduth May 19, 2017

mtias May 23, 2017 •

edited

Loading

aduth May 23, 2017

mtias May 23, 2017

aduth May 23, 2017

aduth May 23, 2017

BE-Webdesign May 23, 2017

aduth May 23, 2017

aduth Jun 2, 2017

jasmussen Jun 5, 2017

jasmussen commented Jun 5, 2017

mtias commented Jun 5, 2017

aduth commented Jun 5, 2017

aduth left a comment

aduth Jun 7, 2017

jasmussen Jun 7, 2017

aduth Jun 7, 2017

aduth Jun 7, 2017

jasmussen Jun 7, 2017

aduth Jun 7, 2017

jasmussen Jun 7, 2017

jasmussen commented Jun 7, 2017

aduth commented Jun 7, 2017

jasmussen commented Jun 8, 2017

jasmussen commented Jun 8, 2017

mtias commented Jun 8, 2017

Address "wide" alignment programatically #812

Address "wide" alignment programatically #812

Conversation

mtias commented May 16, 2017 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtias May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Jun 5, 2017

mtias commented Jun 5, 2017

aduth commented Jun 5, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Jun 7, 2017

aduth commented Jun 7, 2017

jasmussen commented Jun 8, 2017

jasmussen commented Jun 8, 2017

mtias commented Jun 8, 2017

mtias commented May 16, 2017 •

edited

Loading

mtias May 23, 2017 •

edited

Loading