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.

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

Add option to resize Cover Block #17143

Merged
merged 7 commits into from
Aug 26, 2019
Merged

Add option to resize Cover Block #17143

merged 7 commits into from
Aug 26, 2019

Conversation

senadir
Copy link
Contributor

@senadir senadir commented Aug 22, 2019

Description

closes #17113
This PR adds resizing to Cover block & an option to manually select the value.

after #17113 is merged, I need to rebase this PR around it.

How has this been tested?

  • create a new cover block
  • resize
  • save

Screenshots

cover-resize

@senadir senadir added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Enhancement A suggestion for improvement. labels Aug 22, 2019
@senadir senadir requested a review from jasmussen August 22, 2019 13:07
@jasmussen
Copy link
Contributor

MY GOLLY!

I don't have time to review this today, will look tomorrow. But I think @kjellr wants to look at this.

Even from the GIF it also seems clear that we'll want to adjust how the sibling inserter works here. In a related case, a good friend of mine suggested that maybe we adjust the sibling inserter (the plus that appears on hover) to not appear until 2 seconds has passed, something in that vein. Another option is to disable it when the block above has resize handles. Or is selected. There are a few ways to go here, but it feels like a key issue to solve before we roll out such a vertical handle.

@senadir
Copy link
Contributor Author

senadir commented Aug 22, 2019

@mapk have a PR (#17136) related to this problem so I didn't want to address it here but yeah, the bottom resizer is being shadowed by the inserter.
I'm not sure about the delay, delays works great in visual animations but not functional animations, I think the spacing should be fine?

@mtias
Copy link
Member

mtias commented Aug 22, 2019

There's some more ideas here for the sibling inserter. I don't think spacing alone is enough: #16646 (comment)

@ZebulanStanphill
Copy link
Member

Wouldn't it make more sense for the drag handle and input to control the min-height of the block, to prevent the inner content from overflowing out of the container when squeezed down to small widths?

packages/block-library/src/cover/save.js Outdated Show resolved Hide resolved
packages/block-library/src/cover/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/cover/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/cover/edit.js Outdated Show resolved Hide resolved
@oxyc
Copy link
Member

oxyc commented Aug 23, 2019

Wouldn't this be better if it changed the min-height rather than height? Imagine mobile viewports where the content spans more lines because of less horizontal space.

Also, does the COVER_MIN_HEIGHT value need to be hardcoded? I'm already using this block on some sites with a much lower min-height value. Usually because they're presented in columns. I guess that means if a user so much as touches the resizer, it will get a height inline style value that's impossible to remove.

@senadir
Copy link
Contributor Author

senadir commented Aug 23, 2019

Wouldn't this be better if it changed the min-height rather than height? Imagine mobile viewports where the content spans more lines because of less horizontal space.

Also, does the COVER_MIN_HEIGHT value need to be hardcoded? I'm already using this block on some sites with a much lower min-height value. Usually because they're presented in columns. I guess that means if a user so much as touches the resizer, it will get a height inline style value that's impossible to remove.

those are some very good points @oxyc that I haven't consider.

A second implementation that I didn't upload yet, changes height to min-height, as for the hardcoded min height, I guess I should differ to something more absolute, 20 or 50px sounds reasonable to not get a invisible block and still over wide arrangements.

after giving the code a second look, and with @jorgefilipecosta review, I guess I was just too damn tired yesterday 😅

@senadir
Copy link
Contributor Author

senadir commented Aug 25, 2019

I've changed the behavior, 430 is not the min height now but the default one. in the future we might abstract it even more to be a theme option, if there is a better option for now I'm all ears to implement it.

the new min value is arbitrarily set to 50px, no strong feeling toward this value, just seemed small enough to not restrict and big enough so the cover is not invisible.

Also now we change the min height & not the height, should fix a lot of compatibility problems.

the input logic has been heavily inspired (read: literally copied & changed variable names) from the spacer block #14785, thank you @Jackie6 & @talldan for the awesome work solving all of those problems for me before hand.

I think this is ready for a final review

@jasmussen
Copy link
Contributor

Nice work.

Here's what I'm seeing:

resize

When you increase the vertical height, it looks good. When you decrease the vertical height, it appears as if the content inside only "snaps into place" when you release the mouse. Can we do so increasing and decreasing size both work smoothly?

@senadir
Copy link
Contributor Author

senadir commented Aug 26, 2019

@jasmussen you know, this bug bothered me for the past 4 days and I didn't quite knew how to fix, but then you mentioned it and suddenly I knew the source of the problem & how to fix. Please comment more on my problems 😂
fixed in 8fd64f7
resizing-block-fixed

@jasmussen
Copy link
Contributor

Solid work!

@senadir senadir requested a review from mtias August 26, 2019 09:19
@kjellr
Copy link
Contributor

kjellr commented Aug 26, 2019

This looks great. 🎉

Just a quick coordination note: we'll want to make sure that this plays nicely with the 100vh "Fullscreen" option under development in #16738 (cc @retrofox). My guess is that when the fullscreen option is selected, these controls would disappear or be disabled automatically.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Excellent work @senadir 👍 The code changes look good!

Here are some possible improvements I noticed:

  • We are setting a min-height, and it works great on the frontend on the editor thought it looks as if we are setting a height. If the content is bigger than the height, we set the height does not expand. We may need to configure the style generation of resizeable (something similar to what happened with media & text).
  • If I resize a cover block, a min-height will always be set from now on. There is nothing I can do to avoid setting a min-height after applying some resizing operation. We probably need some mechanism to allow a reset.

@jorgefilipecosta jorgefilipecosta added this to the Gutenberg 6.4 milestone Aug 26, 2019
@jorgefilipecosta jorgefilipecosta merged commit 9b0cdf1 into master Aug 26, 2019
@jorgefilipecosta jorgefilipecosta deleted the add/resize-cover branch August 26, 2019 13:23
donmhico pushed a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
* add resize support to cover block

* add height settings to option

* fix issue with setting min height

* fix deleted file

* change from height to minHeight

* refactor how changes are handled

* fix problem with resizing to smaller sizes
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* add resize support to cover block

* add height settings to option

* fix issue with setting min height

* fix deleted file

* change from height to minHeight

* refactor how changes are handled

* fix problem with resizing to smaller sizes
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* add resize support to cover block

* add height settings to option

* fix issue with setting min height

* fix deleted file

* change from height to minHeight

* refactor how changes are handled

* fix problem with resizing to smaller sizes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow "Cover" block to be resizable
8 participants