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

Jump to bottom

Always show "new gallery item" below the gallery #7391

Merged
merged 2 commits into from
Jul 3, 2018
Merged

Always show "new gallery item" below the gallery #7391

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
010a6d4
Always show "new gallery item" below the gallery
caxco93 Jun 20, 2018
08d2fa7
chore: Improve comments and fix whitespace
tofumatt Jul 3, 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
2 changes: 1 addition & 1 deletion core-blocks/gallery/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ class GalleryEdit extends Component {
</li>
) ) }
{ isSelected &&
<li className="blocks-gallery-item">
<li className="blocks-gallery-item has-add-item-button">
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this class name exactly... wouldn't is-selected or something like that be better? Or I guess this is saying it's surrounding the add-item-button, but this has- prefix makes this read to me like it could have or could not have that button.

What about blocks-gallery-add-item-button?

Copy link
Member

Choose a reason for hiding this comment

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

Just noticed that this file doesn't follow coding guidelines for CSS class names: https://github.com/WordPress/gutenberg/blob/master/docs/reference/coding-guidelines.md#naming.

In this case it should be:
.editor-[ directory name ]__[ descendant description ].is-[ modifier description ]
or
.editor-[ directory name ]__[ descendant description ]

I.e.:
classname="core-blocks-gallery__item is-add-button"
or
classname="core-blocks-gallery__add-item-button"

Copy link
Member

Choose a reason for hiding this comment

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

I wish we could lint these somehow, I always forget them as my brain is still adjusting from my old class name habits!

Thanks for catching that. 👍

Copy link
Member

@gziolo gziolo Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It's doable. I think @aduth has implemented something similar for Calypso a long time ago. This rule is following BEM (Blocks, Elements, Modifiers), which is pretty well established standard.

Copy link
Contributor Author

@caxco93 caxco93 Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @tofumatt thanks for the review. I used has-add-item-button instead of is-selected because the latter is a different existing behaviour. Also, yes. A gallery ítem like could or not have that button (this same element type is used to contain the images). The button per se was already 100% width, but its container wasn't. Does this make sense to you guys? @gziolo

<FormFileUpload
multiple
isLarge
Expand Down
7 changes: 7 additions & 0 deletions core-blocks/gallery/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,13 @@
}
}

// Apply max-width to gallery item that contains the add new gallery item button
Copy link
Member

Choose a reason for hiding this comment

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

This comment could be a bit clearer; it's intent is to make the "add new gallery item" button full-width, so let's just say that 😄:

// Make the "Add new Gallery item" button full-width (so it always appears below other items).

.blocks-gallery-item {
&.has-add-item-button {
width: 100%;
}
}

// Apply max-width to floated items that have no intrinsic width
[data-align="left"] &,
[data-align="right"] &,
Expand Down