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

Adopt icon font in debug breakpoints and toolbar #84771

Merged
merged 15 commits into from
Nov 15, 2019

Conversation

miguelsolorio
Copy link
Contributor

@miguelsolorio miguelsolorio commented Nov 14, 2019

Related #78889

This converts all of the breakpoint and toolbar icons to use our new icon font. Nothing should be visibly different as the icons are the same.

One thing I would love to get feedback on is how I modified the ICommandAction to include a new option, iconClassName, and also made the iconLocation parameters optional as these are not needed on the icon fonts. cc @bpasero to check as well (I had to set these parameters as undefined in the registerDebugToolBarItem).

If you could also stress test the breakpoints, I used the mock debug/extension debuggers to ensure everything was accurate to stable.


image

We also introduce a new set of icon color tokens:

  • debugIcon.breakpointForeground
  • debugIcon.breakpointDisabledForeground
  • debugIcon.breakpointUnverifiedForeground
  • debugIcon.breakpointStackframeForeground
  • debugIcon.breakpointStackframeFocusedForeground
  • debugIcon.stepBackForeground

And when configured you can get something like:

image

cc @aeschli for icon color token names

@isidorn
Copy link
Contributor

isidorn commented Nov 14, 2019

The code changes look good.
I tried to test the changes out, however due to conflicts I was unable to do that - I get broken codicons
Screenshot 2019-11-14 at 16 20 33

The one thing I am not sure about is exposing all the different breakpoint foreground colors.
This gives the user flexibility to give different breakpoint types different colors, however we already distrunigsh them with shape and symbols.
The downside of this approach is that if I want to cahnge the breakpoint color now I need to change mulitple colors.
Due to that I suggest to maybe just go with one color that ll breakpoints will share. And that we introduce additional colors only if users request this. Let me know what you think

@miguelsolorio
Copy link
Contributor Author

miguelsolorio commented Nov 14, 2019

I tried to test the changes out, however due to conflicts I was unable to do that - I get broken codicons

If you checkout the branch you should have the correct files, I don't think the patch method for testing PRs works with binary files, which is why the icons don't show up. Can you try again with this method? Would love for you to stress test the breakpoint icons.

The one thing I am not sure about is exposing all the different breakpoint foreground colors.
This gives the user flexibility to give different breakpoint types different colors, however we already distrunigsh them with shape and symbols.

We did this with symbol icons where each symbol got a color token, not sure why it would be different here. Curious to hear what @aeschli thinks as well, I can go either way.

The downside of this approach is that if I want to cahnge the breakpoint color now I need to change mulitple colors.

Actually, all of the breakpoint colors inherit from debugIcon.breakpointForeground so if you set that then all of the other icons will inherit the same color.

@isidorn
Copy link
Contributor

isidorn commented Nov 14, 2019

@misolori great for color inheritence, that makes things easier. However I am always against adding things which we are not even sure users would want. So I do not think these additional colors are needed, until people / extension authors ask for them I would not add them.

I have checked out the PR branch, however your PR has a conflict and I can not build nicely. Can you please resolve the conflict (you will have to do this either way before merging in)?

@miguelsolorio
Copy link
Contributor Author

@isidorn fixed the merge conflicts now

In terms of the various icon colors, I'll wait for @aeschli's feedback as well. I think I was originally in favor of less color tokens but we wanted to make sure to give theme authors the most flexibility.

@isidorn
Copy link
Contributor

isidorn commented Nov 14, 2019

@misolori thanks for fixing the conflicts. I tried this out and this is aweosme work! I am looking forward to you merging this so I no longer have to look at the ugly red breakpoints and turn them to blue.
Adding approved since I tested this quite a bit. Both for various brekapoint icons and for the toolbar.
I would still prefer that we have just one color for breakpoints, though let's wait for @aeschli feedback.

Thanks for doing this!

fyi @weinand

@miguelsolorio
Copy link
Contributor Author

Discussed this in Redmond's standup and everyone was in favor of trimming down the color tokens to single color for all breakpoints (active/red and inactive/gray).

@isidorn
Copy link
Contributor

isidorn commented Nov 15, 2019

Great, once we have that in this PR we can merge it in. Thanks

@miguelsolorio
Copy link
Contributor Author

I've simplified the breakpoint color tokens to:

  • debugIcon.breakpointForeground
  • debugIcon.breakpointDisabledForeground
  • debugIcon.breakpointUnverifiedForeground
  • debugIcon.breakpointStackframeForeground
  • debugIcon.breakpointStackframeFocusedForeground

@miguelsolorio miguelsolorio merged commit 2b96305 into master Nov 15, 2019
@miguelsolorio miguelsolorio deleted the misolori/icon-font-breakpoints branch November 15, 2019 18:38
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants