-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Comments
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 🤔 |
I agree with that and I don't think we can swap this with Maybe what we need here is a |
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. |
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. |
Updated the title of this issue to make the intent clearer :) |
I am interested in contributing .... can you tell me what to do and how to do ????? @annezazu |
👋 @rkg4412 ! Thanks so much! You can see the contributor guide here. The actual change would be here by using this existing |
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. |
I just opened a PR (here). Please let me know if I did any thing wrong or if I could improve upon anything. |
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? |
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: 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: |
Since "Minimum Height" is fairly self explanatory and as @jasmussen mentioned
is there even a need for any sort of helper text? Perhaps this task should be closed? |
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. |
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:
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:
The text was updated successfully, but these errors were encountered: