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

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

Merged
merged 2 commits into from Jul 3, 2018

Conversation

Projects
None yet
5 participants
@caxco93
Contributor

caxco93 commented Jun 20, 2018

Added has-add-item-button class to the corresponding li element.
Added has-add-item-button class css for the desired effect.

Description

Selecting a gallery won't shift the layout presentation to accommodate the extra placeholder item anymore.

Fixes #7300.

How has this been tested?

Ran npm test using the docker-compose setup.

Screenshots

Before:
not_below
After:
always_below

Types of changes

Visual change

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Jun 20, 2018

@tofumatt

I dig it, nice!

Just a small comment tweak and class name change requested from me, but I tested it locally and it works well!

@@ -81,6 +81,13 @@
}
}
// Apply max-width to gallery item that contains the add new gallery item button

This comment has been minimized.

@tofumatt

tofumatt Jun 20, 2018

Member

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).
@@ -264,7 +264,7 @@ class GalleryEdit extends Component {
</li>
) ) }
{ isSelected &&
<li className="blocks-gallery-item">
<li className="blocks-gallery-item has-add-item-button">

This comment has been minimized.

@tofumatt

tofumatt Jun 20, 2018

Member

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?

This comment has been minimized.

@gziolo

gziolo Jun 20, 2018

Member

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"

This comment has been minimized.

@tofumatt

tofumatt Jun 20, 2018

Member

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. 👍

This comment has been minimized.

@gziolo

gziolo Jun 20, 2018

Member

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.

This comment has been minimized.

@caxco93

caxco93 Jun 20, 2018

Contributor

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

@karmatosed karmatosed self-requested a review Jun 20, 2018

@karmatosed

This comment has been minimized.

Member

karmatosed commented Jun 20, 2018

Thanks for looking into this and suggesting @nagayama. [edit] I was going to close but see there's discussion in issue so will add my thoughts there.

@karmatosed karmatosed closed this Jun 20, 2018

@karmatosed karmatosed reopened this Jun 20, 2018

Always show "new gallery item" below the gallery
Added `has-add-item-button` class to the corresponding `li` element.
Added `has-add-item-button` class css for the desired effect.
@karmatosed

Approving based on consensus to get feedback.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 3, 2018

Sweet! 🚢

@tofumatt tofumatt merged commit 9f5c9d2 into WordPress:master Jul 3, 2018

2 checks passed

codecov/project 46.61% (-0.19%) compared to b299f4f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tofumatt tofumatt added this to the 3.2 milestone Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment