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

Consolidate icon styles into AccessibleImage.css and SourceIcon.css #7890

Merged
merged 1 commit into from Feb 11, 2019

Conversation

@fvsch
Copy link
Member

commented Feb 6, 2019

Fixes #7735

This is an alternative to PR #7841 by @jaril, following the methodology described in:
#7841 (comment)

Summary of Changes

  • Change the base style of .img to be 16 by 16px, use --theme-icon-color and mask styles
  • Remove a lot of CSS changing that icon size (bigger or often smaller), mask-size, etc.
  • Remove a lot of CSS used for vertical alignment of small icons
  • Use Flexbox in a few places for better vertical alignement of icons (e.g. in the Sources list)
  • Redo a lot of the icon sizing and spacing in Sources, search-bar and a few places
  • Update a bunch of SVG icons to make sure they have a square 16x16 canvas with the visuals centerd in that canvas (e.g. Vue, Webpack); still some work to do, perhaps in a followup bug
  • Update some SVG icons with icons from Photon and/or other DevTools panels (especially icons that shipped in Firefox 66)
  • Remove inline SVG for the magnifying glass icon; use .img.search instead.
  • Remove a lot of dead CSS code that was targetting SVG elements that were removed in the past few months.
  • While I was fixing some icon spacing issues, fixed a bunch of RTL spacing issues that was happening nearby (e.g. hardcoded left/right margin-left/right etc.)

I know there is a LOT in that pull request, but I don't think there would be much value in trying to break it up in half a dozen PRs. The changes are all somewhat related to deduplicating image styles and making sure images are 16x16 99% of the time and look good with the accompanying content. JS changes are limited to changing classes, icon names, and exceptionally moving an element outside of its parent and into its grandparent container.
I'm prepared to work on follow-up CSS fixes if I've regressed a few specific styles.

The best way to test this PR would be to check it out and take it for a ride, paying extra attention to icons in Sources and in search UIs.

@jaril

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Checked the following components for any major regressions, and couldn't find much apart from the minor ones I list below. Really great job on this - thanks @fvsch!!!

  • src/components/Editor/Footer.css
  • src/components/Editor/Preview.css
  • src/components/Editor/Preview/Popup.css
  • src/components/Editor/SearchBar.js
  • src/components/Editor/Tabs.css
  • src/components/PrimaryPanes/Sources.css
  • src/components/PrimaryPanes/SourcesTreeItem.js
  • src/components/ProjectSearch.css
  • src/components/SecondaryPanes/CommandBar.js
  • src/components/SecondaryPanes/EventListeners.css
  • src/components/SecondaryPanes/SecondaryPanes.css
  • src/components/shared/AccessibleImage.css
  • src/components/shared/Accordion.css
  • src/components/shared/Button/styles/CloseButton.css
  • src/components/shared/Dropdown.css
  • src/components/shared/ManagedTree.css
  • src/components/shared/ResultList.js
  • src/components/shared/SearchInput.js
  • src/components/shared/SourceIcon.css
  • src/components/variables.css

I only ran into a few things:

  • Tab icons' end margins are being overwritten to 0px, where I believe you intended for them to be 4px

screen shot 2019-02-05 at 10 54 03 pm

  • The text within ResultList (file name vs relative URL) seem to be vertically misaligned by a bit

screen shot 2019-02-05 at 10 28 13 pm

  • coffeescript.svg is not displaying properly - I seem to get the error This XML file does not appear to have any style information associated with it.

screen shot 2019-02-05 at 11 02 36 pm

  • Small visual tweaks may be needed for immutable.svg (which is displayed too high) and aframe.svg (which displays too big)

screen shot 2019-02-05 at 10 44 27 pm screen shot 2019-02-05 at 10 44 09 pm

@jasonLaster

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Thank you for the thorough check @jaril!

@fvsch fvsch force-pushed the fvsch:icon-style-consolidation branch from 54f5e27 to 35dabb6 Feb 6, 2019

@fvsch fvsch changed the title Consolidate icon styles into AccessibleImage.css Consolidate icon styles into AccessibleImage.css and SourceIcon.css Feb 6, 2019

@fvsch

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

  • Fixes the issues reported by Jaril
  • Moved all the library icons to images/sources
  • Tentatively changed library icon selectors from a mix of .img.foo and .source-icon.foo to .img[data-source="foo"]. This works well in my tests.
@darkwing

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Great work @fvsch ! Great progress. I spotted a few things:

  1. The HTML icon is blacked out in the dark and light theme:

htmlicons

darkblock

  1. During project search, I see JS files given the same block icon issue:

jsicons

@fvsch

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

Hmm, looks like the .img[data-source="foo"] approach is too risky.
I'll keep the updated SVGs in images/sources, but I'll roll back the selector and SourceIcon.js change.

@fvsch fvsch force-pushed the fvsch:icon-style-consolidation branch 2 times, most recently from cf0d131 to 3fe1406 Feb 7, 2019

@fvsch

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

@darkwing I rolled back the change to SourceIcon classes and selectors, which fixes the issues you reported. We should be good to go.

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

Very nice @fvsch ! I found just one last issue: the close icons ("x") look too big, unless UX decided to increase them:

bigclose

findclose

bigicons

@fvsch

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

@darkwing In Firefox 66 we updated the Plus and Close icons in mozilla-central (so for all the other DevTools panels) to use the Photon icons, which look bigger with their 2px strokes.
So in this PR I took the liberty to bring them over to Debugger.
There is more work to be done on Photonizing icons in Debugger, see:
firefox-devtools/ux#49

That being said, your screenshots look strange to me because the Plus icon should be fatter, and the Close icon should be a tad slimmer. Could it be a browser cache issue? I'll check on my side too.

@fvsch

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

@darkwing It looks like you have the new CSS styles with the old icons displayed.
It should look like this:

screen shot

We do need a bit of padding at the right of the "find in files" search input, though.

@fvsch fvsch force-pushed the fvsch:icon-style-consolidation branch from 3fe1406 to b11f554 Feb 9, 2019

@fvsch

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2019

We do need a bit of padding at the right of the "find in files" search input, though.

Fixed in latest update.

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

Amazing work @fvsch !

@darkwing darkwing merged commit 62f8bb0 into firefox-devtools:master Feb 11, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fvsch fvsch deleted the fvsch:icon-style-consolidation branch Mar 15, 2019

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