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

Debug icons consistency and housekeeping work #45253

Merged
merged 5 commits into from Mar 8, 2018

Conversation

Projects
None yet
2 participants
@chryw
Copy link
Member

chryw commented Mar 8, 2018

@isidorn

  • Re-aligned design, color, and svg code manner for all icons.
  • Changed remove-all.svg back to multi-box clear icon.
  • Removed some "***-dark" icons and let the dark theme css point to the same light theme icon files. This is because some inverted colors are either too bright for dark theme, or almost the same as light theme colors so we don't need extra files.
  • Tweaked some css to define icon sizes. The svg files are responsive to container size.
  • Deleted breakpoint-hint.svg. Let's use the regular breakpoint.svg file but set opacity in css.
  • Tuned up the green color for stackframe-and-breakpoint.svg to match the highlight color
  • Updated the gear icon in both debug and activity bar. image This is the latest version that I have in our master icon repo.

#45253
#45128
#44550
#44849

@isidorn isidorn self-assigned this Mar 8, 2018

@isidorn isidorn added this to the March 2018 milestone Mar 8, 2018

@isidorn isidorn merged commit a306d8f into Microsoft:master Mar 8, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Mar 8, 2018

@chryw great work I merged it in. I now see the following issues:

  • Drag area is misaligned
    screen shot 2018-03-08 at 13 23 46

  • Arrow missing for top stack frame (yellow) and selected stack frame (green)

  • Breakpoint hint decoration takes priority over regular breakpoint decoration. If a user hover over breakpoint it should show that decoration, not the transaparent one

  • Start icon in select box looks too big - compare it to debug toolbar
    screen shot 2018-03-08 at 13 25 16

  • Since you decided to remove the dark theme icons which makes perfect sense I think we can simply remove a lot of css rules. Which you changed to point to the light theme .svg, but instead we can just remove all those rules imho

Let me know if you have time to tackle these issues, if not I can look into fixing some of them. Thanks a lot again!

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Mar 8, 2018

@chryw so we do not rush with the fixes I have decided to revert the PR temporarily again. Sorry about that, it is my mistake, I should have first tried it out before merging.

Also on top of the issues above, here are two more we get at build time:

CSS INLINING IMAGE AT out-build/vs/workbench/parts/debug/browser/media/continue.svg MORE THAN ONCE. CONSIDER CONSOLIDATING CSS RULES
CSS INLINING IMAGE AT out-build/vs/workbench/parts/debug/browser/media/continue-inverse.svg MORE THAN ONCE. CONSIDER CONSOLIDATING CSS RULES
fs.js:646
  return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode);
                 ^

Error: ENOENT: no such file or directory, open 'out-build/vs/workbench/parts/debug/browser/media/breakpoints-activate-inverse.svg'
    at Object.fs.openSync (fs.js:646:18)
    at Object.fs.readFileSync (fs.js:551:33)
    at out-build/vs/css.build.js:317:28
    at out-build/vs/css.build.js:300:12

Let me know if you need help with any of those and I will be happy to.

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Mar 9, 2018

@chryw the missing arrow were my fault and I have mixed this on master.
Now I will merge in your PR and do some polish on top of it so it is in a decent state.
Then I will write here what else is missing :)

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Mar 9, 2018

@chryw merged in your great work. Created this issue #45406 so we can track what else needs to be done. Thanks a lot for your effort

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