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-width to audio block. #11749

Merged
merged 4 commits into from
Nov 12, 2018
Merged

Add min-width to audio block. #11749

merged 4 commits into from
Nov 12, 2018

Conversation

jasmussen
Copy link
Contributor

Fixes #11740.

The audio block has no intrinsic width, and collapses if we don't set an explicit min-width on it. This PR does that.

screenshot 2018-11-12 at 09 16 49

What should the min-width be? In this case I've set it to 240px, which is a nice round number, and seems to be the tiniest width at which the actual controls appear usable in.

It would be a nice enhancement in the future to let the user set the width of the audio block themselves, perhaps in an inspector interface like that for the image block.

Have set no milestone since I'm imagining this will go in as a 4.3RC fix.

Fixes #11740.

The audio block has no intrinsic width, and collapses if we don't set an explicit min-width on it. This PR does that.
@jasmussen jasmussen added the [Type] Bug An existing feature does not function as intended label Nov 12, 2018
@jasmussen jasmussen self-assigned this Nov 12, 2018
@jasmussen jasmussen requested review from youknowriad and a team November 12, 2018 08:36
@youknowriad youknowriad added this to the 4.3 milestone Nov 12, 2018
@@ -4,4 +4,7 @@

.wp-block-audio audio {
width: 100%;

// The audio block has no intrinsic width and needs an explicit min-width to not collapse.
min-width: $break-mobile / 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be applied for the frontend too?

@notnownikki
Copy link
Member

The published post already seems to have some kind of width set, which looks different from the editor, could we make it the same?

Editor:

editor-audio

Published post:

frontend-audio

@youknowriad
Copy link
Contributor

The published post already seems to have some kind of width set, which looks different from the editor, could we make it the same?

There's probably things coming from the theme styles there, we should test with a theme without Gutenberg support and see how it behaves.

@notnownikki
Copy link
Member

we should test with a theme without Gutenberg support and see how it behaves.

Do we know one that definitely doesn't have support? I've tried installing random ones and I see the floated audio block correctly in all of them so far.

@jasmussen
Copy link
Contributor Author

I have a fix, pushing in a second. Great feedback, both.

@jasmussen
Copy link
Contributor Author

So, the feedback was solid. Just pushed a change that addresses all of it, and some issues that popped up as a result of the refactor.

Turns out that the audio element DOES have an intrinsic width. It's 300px, and it's given by the browser. But we set that to 100% to avoid this:

screenshot 2018-11-12 at 10 54 44

The centered caption here is weird, right?

But when we float, then the 100% width collapses. If that 100% width was not applied, then it wouldn't have. I looked into ways to apply the 100% width ONLY to a non-aligned block, but wasn't able to do so without using :not.

So I simply added the 300px width as a min-width on the element level.

Editor:

screenshot 2018-11-12 at 10 53 06

Not-gutenberg-ready-theme:

screenshot 2018-11-12 at 10 53 12

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@notnownikki
Copy link
Member

👍

@jasmussen
Copy link
Contributor Author

Thank you, both. I'll be merging when the checks pass, unless anyone complains.

@jasmussen jasmussen merged commit 567875e into master Nov 12, 2018
@jasmussen jasmussen deleted the fix/audio-block-floats branch November 12, 2018 10:39
@ZebulanStanphill
Copy link
Member

@jasmussen

But when we float, then the 100% width collapses. If that 100% width was not applied, then it wouldn't have. I looked into ways to apply the 100% width ONLY to a non-aligned block, but wasn't able to do so without using :not.

Is this a sign that perhaps there should be alignnone classes applied to blocks with no alignment set?

youknowriad pushed a commit that referenced this pull request Nov 12, 2018
* Add min-width to audio block.

Fixes #11740.

The audio block has no intrinsic width, and collapses if we don't set an explicit min-width on it. This PR does that.

* Update changelog

* Move to style.scss instead.

* Refactor, address feedback.
@jasmussen
Copy link
Contributor Author

Is this a sign that perhaps there should be alignnone classes applied to blocks with no alignment set?

In this specific case, I feel the change to use min-width is a superior solution.

But sure, alignnone could make sense to add. But right now some blocks in the editor do not have the alignment classes, like in this case a floated audio block doesn't have the alignleft/aligncenter/alignright classes. A missing feature that will be implemented at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[4.3. RC1] Audio block disappears when align right or left in Chrome
4 participants