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

Allow setting the height from the placeholder state. #34706

Closed
Tracked by #28385
apeatling opened this issue Sep 9, 2021 · 7 comments · Fixed by #35068
Closed
Tracked by #28385

Allow setting the height from the placeholder state. #34706

apeatling opened this issue Sep 9, 2021 · 7 comments · Fixed by #35068
Assignees
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@apeatling
Copy link
Contributor

No description provided.

@apeatling apeatling added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Enhancement A suggestion for improvement. labels Sep 9, 2021
@ramonjd
Copy link
Member

ramonjd commented Sep 14, 2021

I've had a play with this. Please ignore the layout.

Screen Shot 2021-09-14 at 11 50 07 am

In my mind, we could group the background color/media options in a single row + two columns, with the height control below.

Just noting down some considerations for when I, or someone else, rounds back to properly implement:

  1. Because the block insertion handler fires after background selection, we might want to add a "Create Cover Block" button so that the user can modify the height before creating the block
  2. I noticed that cutting (Cmd + X) the input value cuts the entire placeholder. I think we should find a way to prevent the keyboard events bubbling up (if indeed that's what's happening).

I didn't pay any attention to what's going on in the edit.native.js file of the Cover Block so some work might need to be done in its placeholder too.

@stacimc stacimc assigned stacimc and unassigned stacimc Sep 14, 2021
@apeatling
Copy link
Contributor Author

@jasmussen Any thoughts on a good approach for this, or what Ramon has posted above? A large part of me wants to remove the placeholder entirely with the more intuitive color and image controls we have now via the toolbar.

@ramonjd
Copy link
Member

ramonjd commented Sep 16, 2021

My initial thoughts, now I've had time to think about the experiment, was that the placeholder was throwing up too much of a speed hump to editing. Without seeing the block in the editor, I don't really know which min height is appropriate for my image or whichever background and content I have.

At the very least, I'd skip adding more controls to the placeholder and, as Andy says, continue with improving the block inspector controls.

@jasmussen
Copy link
Contributor

jasmussen commented Sep 17, 2021

Thanks for exploring. I share the instinct of wanting to use the Placeholder component more sparingly in general. They are necessary for blocks that do not function without a specific property set first, such as an embed block, an image, or a maps block that needs an API key. The downside is they look nothing like the end result, and they don't scale well to narrow container contexts.

The cover block has a lot of age attached to it, though, and if feedback suggests the need for a min-height as part of the setup state, it seems worth exploring that. Doing so also wouldn't preclude future explorations to reduce the usage of the Placeholder component where possible/useful.

Would it be possible to add the dimensions panel itself to the inspector sidebar even in the setup state? Along the same lines, it feels like the best interface for setting the min-height is the in-canvas resizable box, like so:

cover setup state

This would actually resize the bordered-box Placeholder component.

@ramonjd
Copy link
Member

ramonjd commented Sep 20, 2021

Would it be possible to add the dimensions panel itself to the inspector sidebar even in the setup state?

It might just possible! Regardless, I think it's a great compromise and worth a shot. Thanks for the suggestion and the design concept image. 🙇

@ramonjd
Copy link
Member

ramonjd commented Sep 22, 2021

Preliminary experimentation is looking promising:

min-height-cover

@jasmussen
Copy link
Contributor

Very cool! One gotcha on this design, though, is that it doesn't work if you want to make the cover smaller 🙈

That speaks to the limitation of the placeholder component itself, though, and is why it's good to use it sparingly. It's likely fine, though, as the height is meant as a min-height and will always be affected by the content inside, and once you pick a color or add an image, you can adjust the size again with the same interface.

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 [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants