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.

Sign up for GitHub

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

Fix image alignment #5209

Closed
wants to merge 3 commits into from
Closed

Fix image alignment #5209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d85f13
The figure element should only be the width of the image it contains.
pento Feb 23, 2018
870106b
No need to have a max-width when aligning, as the width is now being …
pento Feb 23, 2018
2d1ed64
In the editor, right aligned images should be on the right side.
pento Feb 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions blocks/library/image/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,9 @@
margin-right: auto;
}
}

.editor-block-list__block[data-type="core/image"][data-align="right"] {
.wp-block-image {
float: right;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand properly, this fix PR is meant for the frontend, is this really necessary? we just merged #5142 that refactors how float work where the idea was to make floats work in a generic way in the editor not requiring specific handling per block.

Can you explain why do we need this specific handling? right now the block nodes (wp-block-image) are not floated in master but their container is in a generic way to avoid having to do it in each block.

Copy link
Member Author

Choose a reason for hiding this comment

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

On further repo reading, I ran into the same bug as #3945, but it may require more attention to properly address the behaviour that causes #3944.

2 changes: 0 additions & 2 deletions blocks/library/image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,8 +177,6 @@ export const settings = {

if ( width ) {
figureStyle = { width };
} else if ( align === 'left' || align === 'right' ) {
figureStyle = { maxWidth: '50%' };
Copy link
Contributor

Choose a reason for hiding this comment

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

I know there have been discussions about whether to add this or not, not sure about the conclusion here though @jasmussen

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep — mostly I'm in the camp of the fewer inline styles, the better, because themes can't override them without using !important. So we should be very careful before we add inline styles like this.

}

return (
Expand Down
4 changes: 4 additions & 0 deletions blocks/library/image/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
.wp-block-image {
width: fit-content;
}

.wp-block-image figcaption {
margin-top: 0.5em;
color: $dark-gray-300;
Expand Down