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 helper text to Cover Block's min height field #38875

Closed
annezazu opened this issue Feb 17, 2022 · 13 comments
Closed

Add helper text to Cover Block's min height field #38875

annezazu opened this issue Feb 17, 2022 · 13 comments
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Design Needs design efforts. [Type] Enhancement A suggestion for improvement.

Comments

@annezazu
Copy link
Contributor

What problem does this address?

If you add a Cover block and include an embed within it, you cannot make the minimum height smaller than the embed. This makes sense but there's no warning in place to tell you that. This was found as part of the FSE Outreach Program's All Things Media exploration:

Insert a YouTube video or Imgur image. Click on the outer cover block. Adjusting the height on the cover doesn’t work.

What is your proposed solution?

The text is terrible :) but just to show an idea -- we could have a similar warning you might see for contrast:

Screen Shot 2022-02-16 at 8 42 13 PM

@annezazu annezazu added [Type] Enhancement A suggestion for improvement. Needs Design Needs design efforts. [Block] Cover Affects the Cover Block - used to display content laid over a background image [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Feb 17, 2022
@jasmussen
Copy link
Contributor

jasmussen commented Feb 17, 2022

Perhaps there's a deeper question on the minimum height control. Technically it's working perfectly as intended: it's the minimum height, and whatever content is inside is free to expand the container beyond that. But of course that isn't necessarily intuitive for most people who might think of it as a plain height control, perhaps especially since the resize handle in the canvas suggests that to be the case.

I seem to vaguely recall @kjellr mentioning some issues related to height vs. min-height in context of Cover. I wonder if we should explore converting this to just height 🤔

@ntsekouras
Copy link
Contributor

Technically it's working perfectly as intended: it's the minimum height, and whatever content is inside is free to expand the container beyond that

I agree with that and I don't think we can swap this with height. It will create a tons of issues and confusion since everything is responsive and will create scrollbars, might cause issues with the fullscreen toggle, etc...

Maybe what we need here is a help text explaining a bit what min-height is?

@annezazu
Copy link
Contributor Author

Ohh a brief description might help. I still am strangely partial to having a warning of sorts to save the sidebar from getting even longer but that might be difficult to intelligently implement.

@ntsekouras ntsekouras added the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Feb 17, 2022
@justintadlock
Copy link
Contributor

I wonder if we should explore converting this to just height

Mostly just reiterating what @ntsekouras said. That seems like a recipe for a lot of broken use cases currently in the wild and would fundamentally change how the Cover block works. Fixed heights also rarely make sense in web design given that content needs to adjust to the screen size. Maybe there should be helper text or a link that explains what min-height is.

@annezazu annezazu changed the title Add warning for making a Cover block height smaller than embed Feb 24, 2022
@annezazu
Copy link
Contributor Author

Updated the title of this issue to make the intent clearer :)

@rkg4412
Copy link

rkg4412 commented Mar 16, 2022

I am interested in contributing .... can you tell me what to do and how to do ????? @annezazu

@ntsekouras
Copy link
Contributor

👋 @rkg4412 ! Thanks so much! You can see the contributor guide here.

The actual change would be here by using this existing help property of BaseControl.

@loganwisniewski
Copy link

Hi there, I am a first time contributor and saw this was a Good First Issue and still Open. I'd like to use this task as an opportunity to get familiar with the Git workflow required to contribute. I appreciate the contributor guide posted above and plan to contribute this by no later than tomorrow if it's, indeed, alright that I use this task as my first contribution.

@loganwisniewski
Copy link

I just opened a PR (here). Please let me know if I did any thing wrong or if I could improve upon anything.

@loganwisniewski
Copy link

My pull request had some failed tests. The failed tests don't appear to be related to my file change. Is there something I may have done wrong here?

@jasmussen
Copy link
Contributor

Thanks, @loganwisniewski for the PR! Code looks good and the test failures look unrelated, I've restarted them.

I want to voice a general concern with the help text as discussed, specifically that it gives weight to the minimum height feature. Width and min-height controls is something I'd love to see come to more container blocks, including the Group block (see also). Especially for stack and row blocks that feature vertical alignment controls, it will be useful. As those controls expand, they naturally belong together, like so:

group

With several lines of help text, this will not be possible. And ultimately, I still don't think it's needed, as "minimum" implies it already. I would love if we could seek alternatives, for example tooltips:
Screenshot 2022-04-15 at 08 05 59

@loganwisniewski
Copy link

Since "Minimum Height" is fairly self explanatory and as @jasmussen mentioned

And ultimately, I still don't think it's needed, as "minimum" implies it already.

is there even a need for any sort of helper text? Perhaps this task should be closed?

@annezazu
Copy link
Contributor Author

I'm game to close this out and can re-open if further feedback comes in. I do think that the minimum height option, when placed with other tools, will become clearer as well. Currently, having it as a one off dimension control might be part of what's making a helper text feel advantageous.

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 [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Design Needs design efforts. [Type] Enhancement A suggestion for improvement.
6 participants