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

Fix incorrect trash unicode address, and tweak build process #246

Merged
merged 4 commits into from Nov 6, 2017

Conversation

Projects
None yet
2 participants
@jasmussen
Copy link
Collaborator

commented Nov 6, 2017

This PR intends to address regressions found as part of the attempt to merge the new icon font (as built by the grunt script) into 4.9. The full thread is here: https://core.trac.wordpress.org/ticket/41074, but a condensed task list is the following:

  1. Remove WOFF file from WordPress core — we now use WOFF2, and it's embedded in the CSS file
  2. Remove SVG file from WordPress core — we don't use it anymore, and the file generated by Dashicons is a sprite, not a font.
  3. Align CSS coding standard
  4. Fix char positions to match old ones (trash had the wrong placement)
  5. Extra CSS markup to be generated

This PR addresses 4, 5, and partially addresses 3. 1 and 2 have to happen upstream.

It now includes a CSS template file, so we can better tweak the spacing and layout of the files. But it looks like we can't edit url("...") to be url( "..." ) without hacking core files.

I haven't found a way to adjust the order in which icons are output in the CSS file — it's done alphabetically. So I don't know that we can sort the CSS classes.

The trash icon issue seems to have been due to a file being renamed (see 8bd649b) without the codepoint also moving.

To add the extra markup, I edited the CSS template file.

So to summarize: this PR fixes the bugs that came with the build process and makes the CSS files better adhere to WP standards. But it does not (can't) go all the way to be a 100% drop-in replacement.

CC: @ntwb

@jasmussen jasmussen added the bug label Nov 6, 2017

@jasmussen jasmussen self-assigned this Nov 6, 2017

@jasmussen jasmussen requested review from folletto and field2 Nov 6, 2017

@field2 field2 merged commit 130613b into master Nov 6, 2017

@field2

This comment has been minimized.

Copy link
Collaborator

commented Nov 6, 2017

What order should the icons be in, if not alphabetically?

@jasmussen

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 6, 2017

What order should the icons be in, if not alphabetically?

I think just the same as the were, to make comparisons easier, see: https://core.trac.wordpress.org/timeline?from=2017-10-25T07%3A13%3A27%2B02%3A00&precision=second

@field2

This comment has been minimized.

Copy link
Collaborator

commented Nov 6, 2017

There are actually a lot of differences between this dashicons.css file and the one in the WP repo. Would it be better to just manually update the WP css file as we've done in the past, until we can figure out how to resolve the two?

@jasmussen

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 6, 2017

Manual updating is very error prone, so I would suggest we only do that as exceptions.

Also, I don't think we can ever get the built files to match the style of the old file. I'm fact I'm not sure we can ever get any closer than we are in this PR. As such it may be good to make this as one big change, get it done, and then have an easier and less error prone future. Just make sure to test thoroughly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.