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 new block icons in media placeholders #9646

Closed
wants to merge 2 commits into from
Closed

Use new block icons in media placeholders #9646

wants to merge 2 commits into from

Conversation

ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Sep 6, 2018

Description

This PR changes the media placeholders of the following blocks to use the new block icons:

  • Audio
  • Cover Image
  • File
  • Gallery
  • Image
  • Video

This PR changes the placeholders to source the icons from the same place as the main block property, meaning that future icon updates will only have to be applied to one place rather than two.

Closes #9642.

Screenshot

image

@@ -13,12 +13,14 @@ import { createBlobURL } from '@wordpress/blob';

export const name = 'core/audio';

export const icon = <svg viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><path d="M0,0h24v24H0V0z" fill="none" /><path d="m12 3l0.01 10.55c-0.59-0.34-1.27-0.55-2-0.55-2.22 0-4.01 1.79-4.01 4s1.79 4 4.01 4 3.99-1.79 3.99-4v-10h4v-4h-6zm-1.99 16c-1.1 0-2-0.9-2-2s0.9-2 2-2 2 0.9 2 2-0.9 2-2 2z" /></svg>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, did you try leaving this in the attributes and importing like this:

import { settings } from './index.js';

const {
  icon,
} = settings;

Alternately I think you could do something like export const icon = settings.icon;.

Definitely not 100% necessary but it feels a little cleaner to keep the icon in the settings object.

Copy link
Contributor

@caxco93 caxco93 Sep 6, 2018

Choose a reason for hiding this comment

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

Sounds like the best way to me as settings is already being exported

Copy link
Member Author

Choose a reason for hiding this comment

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

@chrisvanpatten I was trying to follow the pattern that other blocks do for defining stuff outside of the settings object, e.g. the supports and schema objects in the Paragraph index.js.

Copy link
Member Author

Choose a reason for hiding this comment

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

But that's a good point, @caxco93. In the Paragraph case, the things being defined outside of the settings object are not being exported. I think I will go ahead and change it according to @chrisvanpatten's suggestion.

@ZebulanStanphill
Copy link
Member Author

Addressed feedback and fixed tests.

@ZebulanStanphill
Copy link
Member Author

@chrisvanpatten It looks like that first method you suggested does not work... I think I will try the second method now.

@@ -54,12 +55,14 @@ const blockAttributes = {

export const name = 'core/cover-image';

const icon = <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M4 4h7V2H4c-1.1 0-2 .9-2 2v7h2V4zm6 9l-4 5h12l-3-4-2.03 2.71L10 13zm7-4.5c0-.83-.67-1.5-1.5-1.5S14 7.67 14 8.5s.67 1.5 1.5 1.5S17 9.33 17 8.5zM20 2h-7v2h7v7h2V4c0-1.1-.9-2-2-2zm0 18h-7v2h7c1.1 0 2-.9 2-2v-7h-2v7zM4 13H2v7c0 1.1.9 2 2 2h7v-2H4v-7z" /><path d="M0 0h24v24H0z" fill="none" /></svg>;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ZebulanStanphill I think you missed flipping this one back :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@chrisvanpatten No, that was intentional, because since cover-image does not have a separate edit.js, I was not sure if I could export settings.icon and then reference it from within the definition of settings itself. Actually, cover-image was part of the reason why I did my initial approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered splitting the edit function of cover-image into a separate file, but I decided that was out-of-scope for this PR. What do you think I should do?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ZebulanStanphill Ah, I see now. I think it makes sense to split it out but it might be worth another reviewer jumping in to give this a sanity check before going too far down that path.

Copy link
Member Author

Choose a reason for hiding this comment

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

@chrisvanpatten This PR has some merge conflicts currently. I have created a PR to split the Cover Image edit function into its file: #10582. Once that is merged, I'll rebase or replace this PR.

@ZebulanStanphill
Copy link
Member Author

Rebased and squashed into 2 commits (to show the difference between my initial method and the current one). I would like some more opinions on which approach is best.

@ZebulanStanphill
Copy link
Member Author

This PR has been replaced by #11788.

@ZebulanStanphill ZebulanStanphill deleted the update/use-new-icons-in-media-placeholders branch November 13, 2018 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants