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

Icons: add new icons #32371

Merged
merged 9 commits into from
Jun 4, 2021
Merged

Icons: add new icons #32371

merged 9 commits into from
Jun 4, 2021

Conversation

keoshi
Copy link
Contributor

@keoshi keoshi commented Jun 1, 2021

Description

The goal with these additions is to extend the icon library, and do so respecting the foundational philosophy and icon architecture. The more comprehensive the core icon library is, the more useful it becomes to block/theme/plugin authors. And the more it becomes a common good, the more cohesive the WordPress experience will be, and more people are willing to contribute back to it.

Additions:

  • add-card: a generic element to add new elements beyond just the block editor.
  • bug/remove-bug: useful in technical contexts such as issues, bug reports, fixes, changelogs, etc.
  • key: to be paired with the lock icons, or for plugins that support licenses.
  • post-author: this one was already in the Figma file but not in code. I'm proposing to include it now with the same changes as Icons: tweak people icon #32354.
  • shield: using the shield metaphor to convey protection against existing threats, applies to site health and plugins in general.

Worth repeating that the idea is to add icons that can be used by anyone, in diverse and distinct contexts, and serve multiple purposes.

Read the official proposal on Make Design.

Screenshot

image

keoshi added 2 commits May 31, 2021 17:11
Adds the following icons:

- `add-card`
- `bug`/`no-bug`
- `key`
- `post-author`
- `security`
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jun 1, 2021
@github-actions
Copy link

github-actions bot commented Jun 1, 2021

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @keoshi! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Thanks so very much for this PR! Your icon work is beautiful, and these all have generic qualities that will be much appreciated by folks leveraging the icon library.

Specifically regarding the icons, thanks for going with this variant of the key. Although I see the "OT" 🙈 — I more consistenly see a simple low-poly solid key that works really well with the simple low-poly solid lock we also have. They are a match, and that's lovely!

I have a personal preference for the solid-head person, as I've also noted in the core chat we had about it. But it's not enough of a preference that we need to block this — if we regret the change in a month, we can always pull that back again. On that note, does this PR include #32354 or does that need to be merged separately?

The shield has a curve at the top which feels so subtle it might not need to be there. Does it look okay with a sharp corner?

I left some comments about the code as well, but this one is really nice, thanks again!

packages/icons/src/index.js Show resolved Hide resolved
packages/icons/src/index.js Outdated Show resolved Hide resolved
packages/icons/src/index.js Outdated Show resolved Hide resolved
packages/icons/src/library/add-card.js Outdated Show resolved Hide resolved
packages/icons/src/library/bug.js Show resolved Hide resolved
packages/icons/src/library/key.js Show resolved Hide resolved
@keoshi keoshi requested a review from ajitbohra as a code owner June 2, 2021 10:31
@jasmussen jasmussen self-requested a review June 2, 2021 10:52
Copy link
Contributor

@jasmussen jasmussen 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 fast work! This is looking good to me, and I see the new post author icon in use for the post author block:

Screenshot 2021-06-02 at 12 52 16

I still prefer the old one 🙈 but only a little. Happy to try this.

I also still think the top corner of the shield should be sharp, not rounded — it looks intentional when big:

Screenshot 2021-06-02 at 12 51 41

But it doesn't look big enough to be intentional when small:

Screenshot 2021-06-02 at 12 51 49

that might be nice to tweak, but approving regardless.

@keoshi
Copy link
Contributor Author

keoshi commented Jun 2, 2021

Hi again, @jasmussen!

Your icon work is beautiful, and these all have generic qualities that will be much appreciated by folks leveraging the icon library.

Truly appreciate it. 🙇‍♂️

I more consistenly see a simple low-poly solid key that works really well with the simple low-poly solid lock we also have. They are a match, and that's lovely!

Agreed! In context, the "OT" won't be noticeable I don't think.

I have a personal preference for the solid-head person, as I've also noted in the core chat we had about it.

Your point was taken in consideration for sure, but there also a few comments for the new shape so I've decided to bring that one in and let people weigh in on the PR. Not sure if it's more actionable than a conversation in Slack or the post on Make Design, but that was my hope.

does this PR include #32354 or does that need to be merged separately?

I've commented on that PR but my preference would be to get that other one merged first and afterwards I'll rebase the current one.

The shield has a curve at the top which feels so subtle it might not need to be there. Does it look okay with a sharp corner?

The subtle curve seems to be noticeable in retina screens so I left it in, but I can simplify if needed.

Thank you so much for all your support, Joen!

@jameskoster jameskoster added the Needs Figma Update Needs an update to Figma for design purposes label Jun 2, 2021
@keoshi
Copy link
Contributor Author

keoshi commented Jun 3, 2021

Update the shape of shield — would appreciate a final review on this and then I'll leave the decision to merge to you. :)

Thanks so much once again!

@keoshi keoshi requested a review from jasmussen June 3, 2021 13:28
@jasmussen
Copy link
Contributor

Screenshot 2021-06-04 at 09 11 54

Screenshot 2021-06-04 at 09 11 59

I think this works better with the other shapes in use, thanks for making that update! I'll restart the checks and see if I can merge when they pass. Thanks for your work here!

@jasmussen
Copy link
Contributor

There were a couple of lint errors (just formatting) that I pushed a fix for. If you use VSCode, you can install the ESLint extension for them to be highlighted. Once the test pass I will land this.

@jasmussen jasmussen merged commit 7274601 into WordPress:trunk Jun 4, 2021
@github-actions
Copy link

github-actions bot commented Jun 4, 2021

Congratulations on your first merged pull request, @keoshi! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 10.9 milestone Jun 4, 2021
@keoshi keoshi deleted the add/new-icons branch June 4, 2021 08:50
@jameskoster
Copy link
Contributor

@keoshi please remember to update and publish the figma library if you didn't already :)

@keoshi
Copy link
Contributor Author

keoshi commented Jun 4, 2021

@jameskoster Done! Thanks for the reminder — I'd added them all, but not published. :)

@paaljoachim
Copy link
Contributor

paaljoachim commented Jun 10, 2021

@keoshi Filipe

Thanks for adding the icons!

Two more icons we could use are....

A new "Convert to regular blocks" icon that can be used instead of the duplication kind of associated look icon currently in use in the Reusable blocks.
Reusable-block-Convert

and

A new "The Move to widget area" icon that can be used instead of the kind of slalom slope icon (hey I am a Norwegian and we are born with skies on our feet..:) currently in use in the new Widget screen toolbars.

Screen Shot 2021-06-10 at 12 08 14

Thank you!
Have a great day!

(Yep this merged PR is probably not the best place to ask for these things....:)

@jameskoster
Copy link
Contributor

@paaljoachim We should probably create new issues for these :D

@jasmussen
Copy link
Contributor

I'd just use a "move" text label instead of the icon, even if I appreciate the quirkiness of it.

@paaljoachim
Copy link
Contributor

paaljoachim commented Jun 10, 2021

We got an issue for the Widgets Move to icon..:)
I just added a comment to it. Suggesting to use the word "Move". As Joen mentioned above.
#32119 (comment)

I will go and create a new issue for the Reusable block toolbar convert icon.

@paaljoachim
Copy link
Contributor

Alrighty here is a Reusable block issue for switching out the icon:
#32586

It could use the @keoshi touch..:)

@keoshi
Copy link
Contributor Author

keoshi commented Jun 10, 2021

Hey, @paaljoachim! Will certainly look into it over the next week.

@paaljoachim
Copy link
Contributor

It would be very helpful with feedback so we can get some movement on the "duplicate folder" look'a'like "Convert to regular blocks" icon for Reusable blocks and the "slalom slope" icon for the Widget screen.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Needs Figma Update Needs an update to Figma for design purposes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants