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.

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

Use block icons in media placeholders #11788

Merged
merged 4 commits into from
Feb 6, 2019
Merged

Use block icons in media placeholders #11788

merged 4 commits into from
Feb 6, 2019

Conversation

ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Nov 13, 2018

Description

Fixes #9642.

This PR changes the media placeholders of the media blocks to use the corresponding icon of each block, instead of using Dashicons. Since the icons are now being used by both the index.js and (if it exists) edit.js of each of these blocks, I have moved these icons into icon.js files for each block, so the icons are not defined twice and can be imported by multiple other modules.

List of blocks this PR affects:

  • Audio
  • Cover
  • File
  • Gallery
  • Image
  • Media & Text
  • Video

I also took the opportunity to reorganize the imports in the files I modified: they are now organized alphabetically, and imports that were previously incorrectly listed under "Internal dependencies" are now listed under "WordPress dependencies".

Screenshot

image

Related issues and PRs

@ZebulanStanphill
Copy link
Member Author

Test failures seem to be completely unrelated.

@ZebulanStanphill
Copy link
Member Author

Tests are passing now. Thanks for restarting them, @earnjam! 🙂

@earnjam
Copy link
Contributor

earnjam commented Nov 13, 2018

They feel just a bit crowded to the text compared to the old icons. Flagged for design feedback

See screenshots for comparison:

screen shot 2018-11-13 at 11 29 50 am screen shot 2018-11-13 at 11 29 09 am

@earnjam earnjam added the Needs Design Feedback Needs general design feedback. label Nov 13, 2018
@melchoyce
Copy link
Contributor

Good catch, @earnjam — the icons definitely need some additional space. Does Gutenberg not automatically provide the correct spacing between icon + label for placeholder blocks? I assumed it would take care of that kind of thing for folks.

@ZebulanStanphill
Copy link
Member Author

@melchoyce I had assumed it would as well. I am using the <BlockIcon /> component. If I don't use it and just use the <SVG> component directly, then the icons grow ridiculously big. Should I be using a component other than the <BlockIcon /> component?

@mtias
Copy link
Member

mtias commented Nov 15, 2018

I assumed it would take care of that kind of thing for folks.

Should be applied at the placeholder component level. Right now it addresses svgs that are dashicons, not any svg. Should be easy, I can look into it tomorrow.

@mtias
Copy link
Member

mtias commented Nov 15, 2018

Fix for it here: #11947

@ZebulanStanphill
Copy link
Member Author

Rebased and resolved merge conflicts.

Thanks for the margin fix PR, @mtias! 😄

@ZebulanStanphill
Copy link
Member Author

Rebased and resolved merge conflicts again.

@karmatosed karmatosed added this to the WordPress 5.0.1 milestone Nov 22, 2018
@youknowriad youknowriad modified the milestones: WordPress 5.0.1, 4.8 Dec 4, 2018
@youknowriad youknowriad modified the milestones: 4.8, 4.9 Dec 19, 2018
@gziolo gziolo added the [Feature] Media Anything that impacts the experience of managing media label Feb 4, 2019
@gziolo gziolo requested a review from Soean February 4, 2019 13:42
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Code wise it looks quite good. I must admit that I really like the fact that icons were moved to their own file. I can perform the more in-depth review as soon as this PR gets ✅ from one of the designers.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with this, @ZebulanStanphill. It's nice to get these icons all synced up.

Design-wise, this looks pretty good. Just a minor note: All of these icons seem just a little bit too low compared to the text. The icons need to be moved up 1px in order to be centered visually:

Current:
screen shot 2019-02-04 at 9 04 59 am

After:
screen shot 2019-02-04 at 9 05 55 am

I have one other question for everyone: should we actually be using an image icon within the Media + Text block here?

screen shot 2019-02-04 at 9 07 33 am

In this case, that placeholder area is for "Media", not specifically to "Media + Text" like the icon implies.

Thanks again!

@melchoyce
Copy link
Contributor

In this case, that placeholder area is for "Media", not specifically to "Media + Text" like the icon implies.

The "Content" on the right is the text placeholder, right?

@ZebulanStanphill
Copy link
Member Author

@kjellr I have now updated the Media & Text block placeholder to use that new icon. 🙂

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

On the code level, all those changes look good. I like that icons were moved to their own file 👍

I'm leaving final ✅ to @melchoyce and @kjellr who have better idea how those icons should integrate :)

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Looks great, thank you @ZebulanStanphill! I still may iterate on that image/video icon a little bit, but that can be handled separately. 👍

screen-shot-2019-02-06-at-9 58 23-am

@kjellr kjellr merged commit a615ce6 into WordPress:master Feb 6, 2019
@ZebulanStanphill ZebulanStanphill deleted the update/media-placeholder-icons branch February 7, 2019 02:27
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Use block icons in media placeholders

* Add braces around BlockIcon components when used as attribute values.

Co-Authored-By: ZebulanStanphill <zebulanstanphill@gmail.com>

* Vertically center content of placeholder label.

* Change Media & Text block placeholder icon
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Use block icons in media placeholders

* Add braces around BlockIcon components when used as attribute values.

Co-Authored-By: ZebulanStanphill <zebulanstanphill@gmail.com>

* Vertically center content of placeholder label.

* Change Media & Text block placeholder icon
This was referenced Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media Anything that impacts the experience of managing media Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants