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

Fix/resizing images by dragging #7376

Merged
merged 9 commits into from Jun 28, 2018

Conversation

Projects
None yet
4 participants
@azaozz
Contributor

azaozz commented Jun 19, 2018

  • Removes the resizing handles at the corners.
  • Adds resizing by dragging the bottom edge of the image.
  • Adds resizing by dragging the left or right edge depending on image alignment and RTL.

See #7375.

@azaozz azaozz requested review from karmatosed and iseulde Jun 19, 2018

@@ -344,6 +344,7 @@ class ImageEdit extends Component {
const ratio = imageWidth / imageHeight;
const minWidth = imageWidth < imageHeight ? MIN_SIZE : MIN_SIZE * ratio;
const minHeight = imageHeight < imageWidth ? MIN_SIZE : MIN_SIZE / ratio;
const isRLT = window.getComputedStyle( document.querySelector( '.editor-block-list__layout' ) ).direction === 'rtl';

This comment has been minimized.

@mtias

mtias Jun 19, 2018

Contributor

Should not be getting state from DOM. Can use selector for it within withSelect:

isRTL: select( 'core/editor' ).getEditorSettings().isRTL,

Then access as prop.isRTL.

This comment has been minimized.

@azaozz

azaozz Jun 20, 2018

Contributor

Right, was looking at that. RTL is a bit of a special case. getComputedStyle() has the advantage of catching both the dir attribute and CSS direction, and dynamically gets the proper state used at the moment.

The dir attribute is usually set the same as the editor setting but in some cases may be reversed (including from the CSS direction). For example the code block in RTL languages should still be LTR. I agree this generally wouldn't apply to image blocks, so using the editor setting should be sufficient.

Also it seems we don't yet have good RTL support. The classic editor gets an additional button in RTL languages that lets the user reverse text direction. We'll probably need to add that in most blocks with RichText, and set the code block to LTR by default.

@mtias mtias added this to the 3.2 milestone Jun 26, 2018

@mtias mtias added the Media label Jun 26, 2018

@@ -346,6 +346,9 @@ class ImageEdit extends Component {
const minWidth = imageWidth < imageHeight ? MIN_SIZE : MIN_SIZE * ratio;
const minHeight = imageHeight < imageWidth ? MIN_SIZE : MIN_SIZE / ratio;
const showRightHandle = ( isRTL && ( align === 'left' || align === 'center' ) || ! isRTL && align !== 'right' );

This comment has been minimized.

@iseulde

iseulde Jun 27, 2018

Member

The logic here feels a bit hard to read. Any way to make it more readable? Guessing it could read something like, if it's aligned centre both handles should show, otherwise show right or left depending on isRTL.

This comment has been minimized.

@iseulde

iseulde Jun 27, 2018

Member

There also seems to be a linting error:

[0] /home/travis/build/WordPress/gutenberg/core-blocks/image/edit.js
[0]   349:40  error  Unexpected mix of '&&' and '||'  no-mixed-operators
[0]   349:86  error  Unexpected mix of '&&' and '||'  no-mixed-operators
[0]   349:86  error  Unexpected mix of '||' and '&&'  no-mixed-operators
[0]   349:97  error  Unexpected mix of '||' and '&&'  no-mixed-operators
[0]   350:39  error  Unexpected mix of '&&' and '||'  no-mixed-operators
[0]   350:59  error  Unexpected mix of '&&' and '||'  no-mixed-operators
[0]   350:59  error  Unexpected mix of '||' and '&&'  no-mixed-operators
[0]   350:70  error  Unexpected mix of '||' and '&&'  no-mixed-operators
[0] 
[0] ✖ 8 problems (8 errors, 0 warnings)

This comment has been minimized.

@azaozz

azaozz Jun 27, 2018

Contributor

Unexpected mix of '&&' and '||' no-mixed-operators

Ha, why am I not seeing these here... Also... "unexpected"... in parentheses..? What is that rule, lol.

Yeah, it is a bit hard to read, can change that to a "proper" if() block. I know, wordy, but oh well...

This comment has been minimized.

@azaozz

azaozz Jun 27, 2018

Contributor

Actually... How do you "unblock" that linter when:

if ( true && ( true || false ) )

would always have a mix of && and ||. This looks like a bug there?

This comment has been minimized.

@iseulde

iseulde Jun 27, 2018

Member

Looks like it's catching the last bit: || ! isRTL && align !== 'right'.

This comment has been minimized.

@iseulde

iseulde Jun 27, 2018

Member

In any case it wouldn't hurt to be a bit more readable :)

This comment has been minimized.

@azaozz

azaozz Jun 27, 2018

Contributor

Yep, an if() block would be better there.

@iseulde

This comment has been minimized.

Member

iseulde commented Jun 27, 2018

Might be good to leave a comment for the next person trying to add corner handles back, pointing at the limitation of the resizable component at this time.

@iseulde

This comment has been minimized.

Member

iseulde commented Jun 27, 2018

Interestingly there's both ew-resize and col-resize for the cursor CSS property, but not sure what the difference is supposed to be.

@iseulde

Looks good to me provided tests pass.

@azaozz

This comment has been minimized.

Contributor

azaozz commented Jun 27, 2018

Ha, are these conflicting linting rules? Can't do

if ( true && ( true || false ) )

because there are && and ||. But you also can't do:

if ( true ) {
    if (  true || false ) 
    .....
}

because of single nested if.

Hmm, is that "linting vs. readability"? :)

@azaozz azaozz merged commit 4318de2 into master Jun 28, 2018

2 checks passed

codecov/project 47.06% (+0.27%) compared to 188b679
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@azaozz azaozz deleted the fix/resizing-images-by-dragging branch Jun 28, 2018

@ZebulanStanphill

This comment has been minimized.

Contributor

ZebulanStanphill commented Jun 28, 2018

@iseulde There are some illustrations here that show the difference between col-resize and ew-resize:

https://developer.mozilla.org/en-US/docs/Web/CSS/cursor

I would say that col-resize implies that dragging will make one thing grow in size and the other shrink (like columns in a limited space).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment