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

Add Dashicon for Tide team #248

Open
danieltj27 opened this Issue Dec 6, 2017 · 21 comments

Comments

Projects
None yet
8 participants
@danieltj27
Copy link

danieltj27 commented Dec 6, 2017

There is a new team on Make.WordPress.org called Tide. I had a look and there doesn't seem to be a suitable dashicon in place to use for that team and the most obvious one is a tidal icon.

I propose that something similar to the Emoji is used for this.

🌊 (Tide Emoji)

@field2

This comment has been minimized.

Copy link
Collaborator

field2 commented Dec 11, 2017

This might be a case where adding another Dashicon is warranted. Let's get feedback on slack #design-dashicons

@MaximSiebert

This comment has been minimized.

Copy link

MaximSiebert commented Dec 11, 2017

👋 hey guys, Maxim from XWP here – I've converted our Tide logo to a dashicon sized SVG for you guys to use, pull request incoming!

tide.svg.zip

@MaximSiebert

This comment has been minimized.

Copy link

MaximSiebert commented Dec 11, 2017

Running into issues when trying to run npm run build. I've included the log file below if anyone can help me out 🙏

2017-12-11T17_29_50_566Z-debug.log

Seems to be caused by the SVG, the build command runs fine on a fresh clone

@field2

This comment has been minimized.

Copy link
Collaborator

field2 commented Dec 15, 2017

I there's something that has to be changed in your svg code. I compared it to the svgs in /source/svg changed it to have some of the same code as the preexisting ones, and the build worked. Unfortunately, whatever I did removed any shapes and tide.svg got converted to a blank square.

@field2

This comment has been minimized.

Copy link
Collaborator

field2 commented Dec 15, 2017

It looks like the build process is particular about the formatting of the svg. I opened the tide icon in Illustrator, sized it to 20x20px , ungrouped it several times as it seems Figma grouped the shapes a few times, and exported the svg with these settings:
image
This shows that we should provide working templates for figma and sketch files, but I'm still in the dark about what the difference is in the code.

jasmussen added a commit that referenced this issue Jan 4, 2018

Try adding Tide icon.
See #248.

This builds and doesn't break the build process. Not sure why the old icon broke it, except for that it was a 14x14px SVG that wasn't a compound shape.

Opening the initial SVG in Illustrator, copying the artwork to a new 20x20px document, making it a compound shape with the pathfinder, made it work.
@field2

This comment has been minimized.

Copy link
Collaborator

field2 commented Jan 4, 2018

I was able to do npm run build with no errors on @jasmussen 's branch. I removed the tide.svg icon from /sources/svg and re-added the one posted to this issue, and tried npm run build and have the errors again.

@field2 field2 closed this in #258 Jan 4, 2018

@field2 field2 reopened this Jan 4, 2018

@field2

This comment has been minimized.

Copy link
Collaborator

field2 commented Jan 4, 2018

Sorry, didn't mean to close this until we decide if adding the icon is warranted

@jeffpaul

This comment has been minimized.

Copy link
Member

jeffpaul commented Jan 8, 2018

@field2 who would decide if adding the icon is warranted?

@jeffpaul

This comment has been minimized.

Copy link
Member

jeffpaul commented Jan 25, 2018

@field2 I'm going to go ahead and confirm that the icon is warranted as Tide is currently using a 'pin' icon on make.wordpress.org and Pascal Casier looking to setup a wordpress.org profile badge that will need a dashicon as well. Assuming we move forward with creating the dashicon, what else is needed?

@field2

This comment has been minimized.

Copy link
Collaborator

field2 commented Jan 26, 2018

OK, I can add it in. We should update https://core.trac.wordpress.org/ticket/41074 to make it official.

@jeffpaul

This comment has been minimized.

Copy link
Member

jeffpaul commented Jan 26, 2018

@field2 are you able to update the patch on 41074 or do we need @ntwb to help refresh the patch for 5.0?

@ntwb

This comment has been minimized.

Copy link
Member

ntwb commented Jan 26, 2018

I've updated #41074 to reference this issue, once Tide is added we can test out the new build process and create a refreshed patch for 41074

@jeffpaul

This comment has been minimized.

Copy link
Member

jeffpaul commented Jan 28, 2018

@ntwb thanks!

@field2 let me know what else is needed to commit the Tide dashicon so we can test the new build process and refresh the patch for 41074.

@jasmussen

This comment has been minimized.

Copy link
Collaborator

jasmussen commented Jan 28, 2018

@jeffpaul I'll be around if need be, I did the initial work on the build process.

The short summary of it, the build process makes it feasible to continue using Dashicons, but the first time the new built files are merged in, the diff will be slightly bigger than it usually is. Quite simply, we can't get the built files to match the old manually edited CSS files perfectly. For example the old files had icons grouped, the new files have icons alphabetized. But it is worth it, we just need to test properly, so good to get in early.

More details in #246 (comment) and the linked trac thread.

@cathibosco

This comment has been minimized.

Copy link

cathibosco commented Jan 22, 2019

@jasmussen @dmaweb
We have simplified the tide dashicon - it is clean and rendered to the correct proportions. Can it be merged into the repo now so it appears here: https://developer.wordpress.org/resource/dashicons/#wordpress

I also left the files in the slack channel a week or so ago - thanks so much!
Tide-simplified.zip

@jasmussen

This comment has been minimized.

Copy link
Collaborator

jasmussen commented Jan 23, 2019

I have a fair bit on my table so I won't be able to make a PR with the updated SVG in the near future.

However we still don't have the new build process font files in WordPress, and 5.1 beta is already out. I wonder if we can still manage to get those in? @ntwb do you have insights or bandwidth into how we get that show on the road?

https://developer.wordpress.org/resource/dashicons/#wordpress

I don't actually know how that page is updated. I could swear @ryelle was involved.

@ryelle

This comment has been minimized.

Copy link
Collaborator

ryelle commented Jan 23, 2019

I don't actually know how that page is updated.

It's a page template in the developer handbook theme: https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/themes/pub/wporg-developer/page-dashicons.php, so it needs a meta ticket/patch.

@ntwb

This comment has been minimized.

Copy link
Member

ntwb commented Jan 23, 2019

However we still don't have the new build process font files in WordPress, and 5.1 beta is already out. I wonder if we can still manage to get those in? @ntwb do you have insights or bandwidth into how we get that show on the road?

I've some time this week and next to start going through Trac to clear my backlog, I'll take a look at this and come up with a plan of attack and leave notes on the appropriate ticket/s

@field2

This comment has been minimized.

Copy link
Collaborator

field2 commented Jan 23, 2019

I can add the new icon in today. It's been a while since I've done one, so bear with me...

@field2

This comment has been minimized.

Copy link
Collaborator

field2 commented Jan 23, 2019

The new icon looks identical to the one in this repo, it's just in a 24x24 artboard instead of a 20x20 one currently there.
There isn't a tide icon in the Gutenberg directory. Should this icon go in there?
I can update the github repo, but I'd prefer if someone else does the trac patch.

@ntwb

This comment has been minimized.

Copy link
Member

ntwb commented Jan 24, 2019

... I'd prefer if someone else does the trac patch.

I should be able to handle that

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