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: Stack on mobile option on Media & Text block. #10969

Merged

Conversation

jorgefilipecosta
Copy link
Member

Description

Closes: #10798

This PR adds a "Stack on mobile" option on media & text block.
If this option is enabled, on mobile instead of the Media & content area appearing side by side they appear stacked up.

How has this been tested?

I checked media & text block continues to work as before.
I checked that of "Stack on mobile" options is true contents appear stacked up on mobile but on large screens continue to appear side by side.

Screenshots

oct-23-2018 20-08-55

@jorgefilipecosta jorgefilipecosta force-pushed the add/stack-on-mobile-option-media-text-block branch from e46c308 to 4ad725c Compare October 23, 2018 19:50
@jorgefilipecosta jorgefilipecosta requested a review from a team October 29, 2018 12:44

.wp-block-media-text > figure > img,
.wp-block-media-text > figure > video {
max-width: unset;
width: 100%;
margin-bottom: -6px;
}

@media (max-width: #{ ($break-small) }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we do the opposite. Build for mobile first and use break-small or break-medium to add the desktop styles. Isn't this possible here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, in this case, we can't follow that approach because custom widths are set using an inline style, and on mobile, we need to overwrite this inline style so we need to use "!important". After having an important set on mobile styles it would be impossible than on the desktop sizes to make it use the inline styles again.
I should have added a comment with this explanation I will add them now.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good answer, but it's also a good question. Can you add an inline comment to explain basically the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @jasmussen I already added a CSS comment there specifying the reason for this, let me know if you find it hard to understand or you would prefer some improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Argh sorry, I missed that. No worries then, you're many steps ahead of me as usual.

@jorgefilipecosta jorgefilipecosta force-pushed the add/stack-on-mobile-option-media-text-block branch from 4ad725c to d927e28 Compare October 29, 2018 15:21
@Soean Soean added the [Block] Media & Text Affects the Media & Text Block label Oct 31, 2018
@jorgefilipecosta jorgefilipecosta requested a review from a team November 1, 2018 21:03
@jasmussen
Copy link
Contributor

Lovely. Based on the GIF, this is shippable behavior. 👍 👍

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 👍

@youknowriad youknowriad added this to the 4.3 milestone Nov 2, 2018
@jorgefilipecosta jorgefilipecosta merged commit cbbf22e into master Nov 2, 2018
@jorgefilipecosta jorgefilipecosta deleted the add/stack-on-mobile-option-media-text-block branch November 2, 2018 10:26
@supernovia
Copy link

Did we decide to take this back out? I noticed stacking on text blocks stopped working.

@AtrumGeost
Copy link

AtrumGeost commented Jan 7, 2019

Hi there,

It does seem this stopped working. I ran a test on a Jurassic Ninja site and got this on mobile:

screen capture on 2019-01-07 at 17-49-36

Site: https://aesthetic-kiwi.jurassic.ninja/media-text-test/

Forget what I said. Missed to enable the "Stack on mobile" option 🤦‍♂️ 😬 . Now it is looking great!

@fabianbrunke
Copy link

How do I change the order of the columns after stacking on mobile?

@tanyaslogos
Copy link

How do I change the order of the columns after stacking on mobile?

I tried using the flex-direction on blocks that have the image aligned on the right but it doesn't work. Is it possible to reverse the columns when stacking for images that are floated to the right?

@yitwail
Copy link

yitwail commented Apr 1, 2019

This is great, but I had to google to get to this git page, so I think it would be better if Stack On Mobile was the default setting when a new Media & Text block is created.

@WallnerK
Copy link

WallnerK commented Apr 8, 2019

This is great, but I had to google to get to this git page, so I think it would be better if Stack On Mobile was the default setting when a new Media & Text block is created.

This! Please make it default.

@designsimply
Copy link
Member

Could you please create new issues for requests for new functionality such as the suggestion to make the stack on mobile behavior the default? Also, it's helpful to include a note about why this is important to you or how you use this feature in your work. Thank you!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media & Text Block: Allow Stacking on Mobile