-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Media & Text: Turn Stack on mobile toggle on by default #14364
Media & Text: Turn Stack on mobile toggle on by default #14364
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify if or where a discussion took place leading to this suggested change?
@@ -64,7 +64,7 @@ const blockAttributes = { | |||
}, | |||
isStackedOnMobile: { | |||
type: 'boolean', | |||
default: false, | |||
default: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken, this would be considered a breaking change, and would invalidate existing Media & Text blocks.
This originated in #14281. I have a feeling it cannot be this easy to update the default, as it will indeed invalidate blocks. Can we make migration somehow simply for this? Also, I think that when the stacking becomes default, we should implement it also as a default in CSS and only add a classname when NOT stacking (opposite logic of the current one). Does that make sense? |
Hi @AmirS thank you for working on this PR.
The blocks were migrated successfully and kept the stack on mobile setting they had.
Unfortunately, it is not possible to implement it as the default on CSS, without changing all the existing blocks that set stack on mobile as false to be stacked on mobile. That is breaking change as we are changing the look of existing blocks even if the user does not open them on the editor. I think this PR should be ready. |
Hey, why was this PR closed? |
Hey @AmirS Could you give us a status update on this PR? Let us know what we can do to help move this PR onward. Is this PR ready for a final review? Thanks. |
Yes, after the required deprecation logic added by @jorgefilipecosta, this PR should be ready for a review. |
Hey @AmirS, the changes here look good but the branch needs to be rebased with the latest changes in |
Hi @noisysocks, the conflicts this PR were really huge and across all the files. I reapplied the change from master following the same logic and regenerated all the media&text fixtures, it seems to be working well and is ready for a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR seems ready, let's merge it. If there is any further comment please add them here and we will iterate.
Description
Fixes #14281.
This PR updates the Media & Text block to be responsive by default.
When new Media & Text block is added, "Stack on mobile" toggle is set to on by default.
How has this been tested?
The Media and Text block is updated to use
true
as a default value forisStackedOnMobile
block attribute.Core__media-text*** fixtures have been regenerated to include
is-stacked-on-mobile
class name for the Media & Text block elements.Screenshots
Checklist: