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

Bug 1451261, complete change to svg icon system, remove FontAwesome lib #5058

Merged
merged 2 commits into from Nov 6, 2018

Conversation

Projects
None yet
3 participants
@schalkneethling
Copy link
Collaborator

schalkneethling commented Oct 30, 2018

Ok, phew. I think this does it. With these changes, FontAwesome (the web font) is not used anywhere except by CKEditor.

I used the following pages to test:

You can create local copies of these using:

make bash
...
./manage.py scrape_document https://developer.mozilla.org/en-US/docs/Sandbox/All-Indicators

I also did a bit of a browse around some of the documentation pages, the landing page etc. All looks to be working as expected. This is rather big and will probably need some time for testing. Please let me know should you find anything that looks to be broken, or not as expected.

Once this is merged, I believe it will take care of the following issues:

There is more cleanup that can be done, but I will leave that for a separate PR. Thank you in advance for your reviews.

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Oct 30, 2018

I will resolve the above conflicts and update the PR soon.

@schalkneethling schalkneethling force-pushed the schalkneethling:bug1451261-change-all-icons-to-svg branch from cbfd4ee to c448df8 Oct 30, 2018

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Oct 30, 2018

This also resolves related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1502813

}
}

.obsolete {
<<<<<<< HEAD

This comment has been minimized.

@jwhitlock

jwhitlock Oct 30, 2018

Member

This merge conflict wasn't resolved, is breaking the build

This comment has been minimized.

@schalkneethling

schalkneethling Oct 31, 2018

Collaborator

Meh, did not see that one. Will fix it up. 😞

This comment has been minimized.

@schalkneethling

schalkneethling Oct 31, 2018

Collaborator

Fixed up.

@schalkneethling schalkneethling force-pushed the schalkneethling:bug1451261-change-all-icons-to-svg branch from c448df8 to 84b87a9 Oct 31, 2018

@jwhitlock

This comment has been minimized.

Copy link
Member

jwhitlock commented Nov 2, 2018

I'll let @hobinjk see if the CSS looks reasonable. Looking at the test pages, there are some differences, which might be minor or may be unintended.

@schalkneethling can you say if these are intentional changes or bugs? For the screenshots, the right side is production and Font Awesome (FA), the left is development and SVG.

In this screenshot, from https://developer.mozilla.org/en-US/docs/Sandbox/All-Indicators:

  • The FontAwesome banners are taller, probably because the icons are larger.
  • The FA thumb icon is an outline with a solid sleeve, while the SVG thumb is filled-in with a solid sleeve
  • The FA note icon is an outline, versus a solid icon for SVG
  • The FA translationInProgress is a planet, while the SVG icon is a pencil

screenshot 2018-11-02 13 46 30

Further down in examples, the FA example has a red frowny face icon and a green smiley icon in the lower right, and the SVG version doesn't:

screenshot 2018-11-02 13 50 40

The "external link" icon is blue in FA, and black in SVG:

screenshot 2018-11-02 13 52 10

non-standard_inline (great name 🙄) is yellow in FA, black in SVG:

screenshot 2018-11-02 13 53 04

obsolete_inline is red in FA, black in SVG:

screenshot 2018-11-02 13 54 34

experimental_inline is blue in FA, black in SVG:

screenshot 2018-11-02 13 55 10

System Messages have clock and frowny face icons in FA, and no icons in SVG:

screenshot 2018-11-02 13 55 57

The redirect icon is yellow-brown in FA, black in SVG:

screenshot 2018-11-02 13 56 47

Moving on to https://developer.mozilla.org/en-US/docs/Sandbox/Indicators, there's a single difference. In FA, the "Mailing list/Newsgroup" and "RSS feed" buttons have external link icons, and they are gone in SVG:

screenshot 2018-11-02 13 59 08

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 5, 2018

The FontAwesome banners are taller, probably because the icons are larger.

Yup

The FA thumb icon is an outline with a solid sleeve, while the SVG thumb is filled-in with a solid sleeve

Intentional

The FA note icon is an outline, versus a solid icon for SVG

Intentional

The FA translationInProgress is a planet, while the SVG icon is a pencil

Intentional

Further down in examples, the FA example has a red frowny face icon and a green smiley icon in the lower right, and the SVG version doesn't:

The "external link" icon is blue in FA, and black in SVG:

Intentional

non-standard_inline (great name 🙄) is yellow in FA, black in SVG:

Not entirely intentional. Makes sense to couple it with a color when used standalone.

obsolete_inline is red in FA, black in SVG:

Not entirely intentional. Makes sense to couple it with a color when used standalone.

experimental_inline is blue in FA, black in SVG

Not entirely intentional, but I believe it works.

The redirect icon is yellow-brown in FA, black in SVG:

Intentional. I do not believe we need an additional color queue, as the box is already conveying the color.

System Messages have clock and frowny face icons in FA, and no icons in SVG:
Moving on to developer.mozilla.org/en-US/docs/Sandbox/Indicators, there's a single > difference. In FA, the "Mailing list/Newsgroup" and "RSS feed" buttons have external link icons, and they are gone in SVG:

Not intentional. This worked prior to open the PR, will have to see what it is missing.

Thanks for the review @jwhitlock

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 5, 2018

System Messages have clock and frowny face icons in FA, and no icons in SVG:
Moving on to developer.mozilla.org/en-US/docs/Sandbox/Indicators, there's a single
difference. In FA, the "Mailing list/Newsgroup" and "RSS feed" buttons have
external link icons, and they are gone in SVG:

Not intentional. This worked prior to open the PR, will have to see what it is missing.

Turns out Firefox only supports base64 encoded strings.

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 5, 2018

I now remember the reason non-standard_inline and obsolete_inline is black. The macro does not add anything to "highlight" it as being standalone and inline. Therefore I cannot just set .icon-trash to be red, as all instances of that class will then be red.

We therefore have two choices:

  1. Leave it black
  2. Update the relevant macros to add a class such as standalone-inline or inline so that I can treat these differently.

Thoughts @jwhitlock?

@jwhitlock

This comment has been minimized.

Copy link
Member

jwhitlock commented Nov 5, 2018

We had a chance to talk about this over, and I think we'd like to get the KumaScript macro changes merged first or at least in the same deploy, so we don't lose colors on some inline indicators. @schalkneethling is this what you remember as well?

Other items we're changing and see if anyone complains:

  • Banner heights, larger icons
  • Icon change for the thumb and note icons
  • translationInProgress changes from a planet to a pencil
  • Lose smile and frown from example-good and example-bad code samples
  • External link icons turn from blue to black
  • Experimental inline beaker goes from blue to black
  • Redirect icon goes from brown to black

And, could still use 👀 and a 👍 from @hobinjk.

@jwhitlock
Copy link
Member

jwhitlock left a comment

See #5058 (comment) for requested changes.

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 6, 2018

Lose smile and frown from example-good and example-bad code samples

We will not loose these, the fix for that will be in the coming update of the PR. Thanks @jwhitlock

@schalkneethling schalkneethling force-pushed the schalkneethling:bug1451261-change-all-icons-to-svg branch from 84b87a9 to 1541154 Nov 6, 2018

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 6, 2018

@jwhitlock Updated, and also opened relevant PR on the KumaScript side:
mdn/kumascript#981

@jwhitlock

This comment has been minimized.

Copy link
Member

jwhitlock commented Nov 6, 2018

With the current branch plus mdn/kumascript#981 (for screenshots, left is this PR / SVG, right is production / Font Awesome, standard DPI screen):

screenshot 20 screenshot 2018-11-06 09 25 08 18-11-06 09 00 00

screenshot 2018-11-06 09 20 20

screenshot 2018-11-06 09 25 08

  • TopicBox - External icons are back, and a bit sharper in the PR than in production.

screenshot 2018-11-06 09 36 37

  • example-good and example-bad code samples - Green smile and red frown in lower right corners

screenshot 2018-11-06 09 38 03

I've also looked at them on a high DPI display, it looks similar.

These are the changing items:

  • Banner heights, larger icons
  • Icon change for the thumb and note icons
  • translationInProgress changes from a planet to a pencil
  • External link icons turn from blue to black
  • Experimental inline beaker goes from blue to black
  • Redirect icon goes from brown to black
@jwhitlock
Copy link
Member

jwhitlock left a comment

With the latest changes, augmented with the KumaScript changes, this PR is ready. I'd like to add tests on the KS side, but I don't know if I'll have bandwidth to make that a blocker, and they could be done later.

@jwhitlock
Copy link
Member

jwhitlock left a comment

I think we shouldn't block on @hobinjk's review, but these are some obvious issues

Show resolved Hide resolved jinja2/includes/icons/arrows/caret-left.svg Outdated
Show resolved Hide resolved jinja2/includes/icons/arrows/caret-right.svg Outdated
Show resolved Hide resolved jinja2/includes/icons/arrows/chevron-left.svg Outdated
@hobinjk

hobinjk approved these changes Nov 6, 2018

Copy link
Collaborator

hobinjk left a comment

Sorry, forgot to approve this earlier. I didn't find any issues then and I only found one minor nit looking over this now


$icon-green-smile: 'data:image/svg+xml;base64,PHN2ZyBzdHlsZT0iZmlsbDogIzRkOWYwYzsiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgd2lkdGg9IjI0IiBoZWlnaHQ9IjI4Ij48cGF0aCBkPSJNMTcuNzE5IDE2Ljc5N0MxNi45MzggMTkuMzEzIDE0LjY0MSAyMSAxMiAyMXMtNC45MzctMS42ODctNS43MTktNC4yMDNjLS4xNzItLjUzMS4xMjUtMS4wNzguNjU2LTEuMjUuNTE2LS4xNzIgMS4wNzguMTI1IDEuMjUuNjU2QzguNzAzIDE3Ljg3NSAxMC4yNSAxOSAxMiAxOXMzLjI5Ny0xLjEyNSAzLjgxMy0yLjc5N2ExLjAwNiAxLjAwNiAwIDAgMSAxLjI2Ni0uNjU2Ljk5NC45OTQgMCAwIDEgLjY0MSAxLjI1ek0xMCAxMGMwIDEuMTA5LS44OTEgMi0yIDJzLTItLjg5MS0yLTIgLjg5MS0yIDItMiAyIC44OTEgMiAyem04IDBjMCAxLjEwOS0uODkxIDItMiAycy0yLS44OTEtMi0yIC44OTEtMiAyLTIgMiAuODkxIDIgMnptNCA0YzAtNS41MTYtNC40ODQtMTAtMTAtMTBTMiA4LjQ4NCAyIDE0czQuNDg0IDEwIDEwIDEwIDEwLTQuNDg0IDEwLTEwem0yIDBjMCA2LjYyNS01LjM3NSAxMi0xMiAxMlMwIDIwLjYyNSAwIDE0IDUuMzc1IDIgMTIgMnMxMiA1LjM3NSAxMiAxMnoiLz48L3N2Zz4=';

$icon-red-frown: 'data:image/svg+xml;base64, PHN2ZyBzdHlsZT0iZmlsbDogI2U2NjQ2NTsiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgd2lkdGg9IjI0IiBoZWlnaHQ9IjI4Ij48cGF0aCBkPSJNMTcuNzE5IDE5LjIwM2EuOTkzLjk5MyAwIDAgMS0uNjQxIDEuMjUgMS4wMDcgMS4wMDcgMCAwIDEtMS4yNjYtLjY1NkMxNS4yOTYgMTguMTI1IDEzLjc0OSAxNyAxMS45OTkgMTdzLTMuMjk3IDEuMTI1LTMuODEzIDIuNzk3Yy0uMTcyLjUzMS0uNzM0LjgyOC0xLjI1LjY1Ni0uNTMxLS4xNzItLjgyOC0uNzE5LS42NTYtMS4yNUM3LjA2MSAxNi42ODcgOS4zNTggMTUgMTEuOTk5IDE1czQuOTM3IDEuNjg3IDUuNzE5IDQuMjAzek0xMCAxMGMwIDEuMTA5LS44OTEgMi0yIDJzLTItLjg5MS0yLTIgLjg5MS0yIDItMiAyIC44OTEgMiAyem04IDBjMCAxLjEwOS0uODkxIDItMiAycy0yLS44OTEtMi0yIC44OTEtMiAyLTIgMiAuODkxIDIgMnptNCA0YzAtNS41MTYtNC40ODQtMTAtMTAtMTBTMiA4LjQ4NCAyIDE0czQuNDg0IDEwIDEwIDEwIDEwLTQuNDg0IDEwLTEwem0yIDBjMCA2LjYyNS01LjM3NSAxMi0xMiAxMlMwIDIwLjYyNSAwIDE0IDUuMzc1IDIgMTIgMnMxMiA1LjM3NSAxMiAxMnoiLz48L3N2Zz4=';

This comment has been minimized.

@hobinjk

hobinjk Nov 6, 2018

Collaborator

nit: inconsistent spacing (I think it's meant to align the PHNs but it falls apart pretty quickly)

@schalkneethling schalkneethling force-pushed the schalkneethling:bug1451261-change-all-icons-to-svg branch from 1541154 to e62d849 Nov 6, 2018

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 6, 2018

Thanks @jwhitlock and @hobinjk - Updated.

@jwhitlock jwhitlock merged commit 6c1e253 into mozilla:master Nov 6, 2018

1 of 2 checks passed

security/snyk - package.json (mdn) 1 new, high severity vulnerable dependency path
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

joshJarr added a commit to potatolondon/kuma that referenced this pull request Nov 12, 2018

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