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 min-height to media & text Block #32325

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

devfle
Copy link
Contributor

@devfle devfle commented May 28, 2021

Description

Fixes #31811
I have added the possibility to assign a min-height in the media and text block.
I have oriented myself here to the cover block. It is my first block customization, I hope I did everything right :)

How has this been tested?

I tested the feature on my wp-env test environment with the latest version of Gutenberg.
I also tested the changes in the most popular browsers (Safari, Chrome, Firefox).

Screenshots

media_text.mov

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@devfle devfle requested a review from ajitbohra as a code owner May 28, 2021 15:45
@devfle
Copy link
Contributor Author

devfle commented Jun 7, 2021

Hi @ajitbohra can you take a look :) ?

@MaggieCabrera MaggieCabrera added [Block] Media & Text Affects the Media & Text Block [Type] Enhancement A suggestion for improvement. labels Jul 30, 2021
@devfle devfle force-pushed the feature/add-min-height-control-to-media-text-block branch from 1bf3dce to 24c9404 Compare August 2, 2021 15:36
@devfle devfle force-pushed the feature/add-min-height-control-to-media-text-block branch from 24c9404 to f247eea Compare September 1, 2021 08:38
@devfle
Copy link
Contributor Author

devfle commented Sep 1, 2021

Hi @MaggieCabrera ,

could you possibly add more reviewers? Updating the branch every week and testing whether everything still works takes time.

Thanks :)

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Hi @devfle 👋

Nice work putting this together! Thank you.

The new min-height functionality works well in my testing. I've added a few comments and questions below.

Overall, I think this is another use case for the proposed min-height block support. To date we'd have explicit uses of min-height in the following blocks:

  • Media & Text
  • Cover
  • File (as preview height)

Other potential blocks that could use min-height might be:

  • Group
  • Columns

I'd be interested to hear @ramonjd and @youknowriad's thoughts as to whether this additional use case moves the needle on adding min-height block support.

Thanks again for all your effort on this PR!

export const DEFAULT_MEDIA_SIZE_SLUG = 'full';
export const CSS_UNITS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

There has been a shift away from hard-coding CSS units. You can instead use the helpful useCustomUnits hook to handle this.

@@ -366,4 +371,98 @@ export default [
);
},
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need a new deprecation here if we are only adding a min-height style to the existing markup? The previous version's markup would still be valid correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I was able to remove it.

@@ -290,6 +298,28 @@ function MediaTextEdit( { attributes, isSelected, setAttributes } ) {
</PanelBody>
);

const dimensions = (
<PanelBody title={ __( 'Dimensions' ) }>
Copy link
Contributor

Choose a reason for hiding this comment

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

The addition of the “Dimensions” panel could be problematic if padding or margin block support is adopted for this block as it also adds “Dimensions”. The cover block actually has this issue at the moment and there are a couple of PRs open addressing it.

There is also the proposed min-height block support that might reduce the changes needed here to toggling a support flag in the block.json plus the native changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The best idea would then probably be to wait until these things are done?

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be easiest to hold fire until a decision is made on which direction we all wish to take at least.

Whether we choose to inject the ad hoc control within the proposed dimensions panel slot (as per the Cover block) or go with the min-height block support, some changes will be needed.

If there's no rush we can wait and only have to do the job once 🙂

@ramonjd
Copy link
Member

ramonjd commented Sep 2, 2021

I'd be interested to hear @ramonjd and @youknowriad's thoughts as to whether this additional use case moves the needle on adding min-height block support.

Thanks for the ping, and for experimenting with the media block and min-height support.

I think making min-height available to the right blocks will provide more power and flexibility to our layouts.

The Media and Text Block, in my mind, is a valid application of min-height if the goal is for greater control over spacing within the block.

It's something that I can envisage padding control also achieving however.

Are there specific design scenarios where min-height is the best option? If so, it would help bolster the case for moving height and min-height controls along.

@aaronrobertshaw
Copy link
Contributor

Are there specific design scenarios where min-height is the best option?

Wouldn't min-height be the better option when you want to crop the image to fill the entire column?

Screen Shot 2021-09-02 at 12 52 13 pm

When viewed at large viewports and the text content doesn't wrap as tall, you'd likely still want to ensure a minimum height for the image side of things.

@ramonjd
Copy link
Member

ramonjd commented Sep 2, 2021

Wouldn't min-height be the better option when you want to crop the image to fill the entire column?

That's a good point. 👍 I was testing using a video.

@devfle devfle force-pushed the feature/add-min-height-control-to-media-text-block branch from f247eea to 56982e0 Compare September 7, 2021 08:33
remove deprecation and add new unit function

remove husky script
@devfle devfle force-pushed the feature/add-min-height-control-to-media-text-block branch from 9e44f39 to 6c9f583 Compare September 7, 2021 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Media & Text Affects the Media & Text Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media & Text: Add minimum height setting
4 participants