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

Tiled gallery & Slideshow: use BlockIcon component in mediaPlaceholders #12160

Merged
merged 3 commits into from
Apr 26, 2019
Merged

Tiled gallery & Slideshow: use BlockIcon component in mediaPlaceholders #12160

merged 3 commits into from
Apr 26, 2019

Conversation

simison
Copy link
Member

@simison simison commented Apr 25, 2019

Small polish to media placeholder icons. Mostly to remove a CSS hack.

Before

Screenshot 2019-04-25 at 21 02 15

After

Screenshot 2019-04-25 at 20 34 57

Changes proposed in this Pull Request:

  • Use Slideshow's own icon in media placeholder
  • Use BlockIcon component for spacing, instead of custom CSS. The component has been there for a while already and when Tiled gallery was implemented, it either didn't exist or it wasn't documented well at that point.

Testing instructions:

  • Build extensions yarn build-extensions
  • Insert Tiled gallery and Slideshow blocks to the post
  • Ensure no visual differences in placeholder's icon placement/spacing/size
  • Slideshow block has its own icon now

Proposed changelog entry for your changes:

—

@simison simison added [Feature] Tiled Gallery A different way to display image galleries on your site, in different organizations and shapes. [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Pri] Low [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Slideshow labels Apr 25, 2019
@simison simison added this to the 7.3 milestone Apr 25, 2019
@simison simison requested review from sirreal and a team April 25, 2019 18:03
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello simison! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D27433-code before merging this PR. Thank you!

@simison simison changed the title Tiled gallery; clean out some CSS and use BlockIcon component Tiled gallery & Slideshow: use BlockIcon component in mediaPlaceholders Apr 25, 2019
simison
simison commented Apr 25, 2019
View reviewed changes
extensions/blocks/tiled-gallery/editor.scss
@@ -106,19 +106,10 @@

// Hide captions and upload buttons in style picker preview
.editor-block-preview__content & {
/* @TODO Caption has been commented out */
// figcaption,
Copy link
Member Author

@simison simison Apr 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Unrelated leftover from #11980 that should be removed. Convenient to get it removed here.

@jetpackbot
Copy link
Collaborator

jetpackbot commented Apr 25, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: May 7, 2019.
Scheduled code freeze: April 30, 2019

Generated by 🚫 dangerJS against 33bd2f3

@matticbot
Copy link
Contributor

simison, Your synced wpcom patch D27433-code has been updated.

sirreal
sirreal approved these changes Apr 25, 2019
View reviewed changes
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

Good stuff 👍

The icon is slightly different from what is currently in core blocks (20px, not wrapped in BlockIcon) but matches current Gutenberg.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Apr 26, 2019
jeherve
jeherve approved these changes Apr 26, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to work. 👍

@simison simison merged commit d7c7af7 into master Apr 26, 2019
@simison simison deleted the update/tiled-gallery-blockicon branch April 26, 2019 14:04
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Apr 26, 2019
kraftbj added a commit that referenced this pull request Apr 26, 2019
@kraftbj
CL: #12160
88d8534
kraftbj added a commit that referenced this pull request Apr 30, 2019
@kraftbj
CL: #12160
49d23e3
@fgiannar fgiannar mentioned this pull request Jun 22, 2020
Merged
@jeherve jeherve mentioned this pull request Feb 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

@sirreal sirreal sirreal approved these changes

Assignees
No one assigned
Labels
[Block] Slideshow [Feature] Tiled Gallery A different way to display image galleries on your site, in different organizations and shapes. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Pri] Low Touches WP.com Files
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
6 participants
@simison @matticbot @jetpackbot @jeherve @sirreal @kraftbj