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

Fix/resizing images by dragging #7376

Merged
merged 9 commits into from
Jun 28, 2018
Merged

Fix/resizing images by dragging #7376

merged 9 commits into from
Jun 28, 2018

Conversation

azaozz
Copy link
Contributor

@azaozz azaozz commented Jun 19, 2018 •
edited
Loading

  • 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.

mtias
mtias reviewed Jun 19, 2018
View reviewed changes
core-blocks/image/edit.js Outdated
@@ -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';
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@azaozz azaozz Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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 [Feature] Media Anything that impacts the experience of managing media label Jun 26, 2018
ellatrix
ellatrix reviewed Jun 27, 2018
View reviewed changes
core-blocks/image/edit.js Outdated
@@ -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' );
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ellatrix ellatrix Jun 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

@azaozz azaozz Jun 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@ellatrix
Copy link
Member

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.

@ellatrix
Copy link
Member

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

ellatrix
ellatrix approved these changes Jun 27, 2018
View reviewed changes
Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Looks good to me provided tests pass.

@azaozz
Copy link
Contributor Author

azaozz commented Jun 27, 2018 •
edited
Loading

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
@azaozz azaozz mentioned this pull request Jun 28, 2018
Closed
@azaozz azaozz deleted the fix/resizing-images-by-dragging branch June 28, 2018 18:37
@ZebulanStanphill
Copy link
Member

@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).

@ZebulanStanphill ZebulanStanphill mentioned this pull request Jun 28, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtias mtias mtias left review comments

@ellatrix ellatrix ellatrix approved these changes

@karmatosed karmatosed Awaiting requested review from karmatosed

Assignees
No one assigned
Labels
[Feature] Media Anything that impacts the experience of managing media
Projects
None yet
Milestone
3.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@azaozz @ellatrix @ZebulanStanphill @mtias