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

Update profiles badge styling #174961

Merged
merged 3 commits into from Feb 23, 2023
Merged

Update profiles badge styling #174961

merged 3 commits into from Feb 23, 2023

Conversation

daviddossett
Copy link
Contributor

Ref #166983

I think the new profiles badge location is a great start. However I think the styling could be improved to feel more at home in VS Code. This PR does the following:

  • Uses badge background and foreground colors instead of border
  • Uses invisible border to separate badge element from icon background
  • Other minor CSS tweaks

I'm using the existing badge colors to ensure we aren't creating yet another badge format. Note that these are different from the (typically blue in default themes)activityBar-badge-background and foreground.

Current

CleanShot 2023-02-21 at 09 24 24@2x

New

CleanShot 2023-02-21 at 09 28 40@2x

@sandy081 @esonnino

@isidorn
Copy link
Contributor

isidorn commented Feb 21, 2023

These looks good to me -> approved.
I wonder if the foreground colour is too strong and might "scream for attention". Though I will get a better perception once I try it out.

Let's wait to hear @sandy081 thoughts before we merge it in.

isidorn
isidorn previously approved these changes Feb 21, 2023
@daviddossett
Copy link
Contributor Author

daviddossett commented Feb 21, 2023

Yes, the foreground is somewhat challenging since it depends on the theme. Some use higher or lower contrast values for badge.foreground. Using activityBar.inactiveForeground most often results in something that won't be accessible.

@sandy081 sandy081 self-requested a review February 21, 2023 21:20
@sandy081
Copy link
Member

image

I think this badge is asking for too much of attention because of its background colour. IMO, the profile badge shall just give info and it shall not be same as the activity badge because the activity badge is meant for seeking user attention. If you ask me, this need not be thought as a badge but as an indicator.

Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided in the comment

@daviddossett
Copy link
Contributor Author

@sandy081 I think that problem is exaggerated by the v2 themes. I discussed with @esonnino offline that I will adjust those themes to differentiate between badge.foreground (grey in most default themes) and activityBar.badgeForeground (blue in most default themes).

Regardless of color choices—which we can change—I don't think the border with transparent background is the way to go here. It really sticks out to my eye.

@sandy081
Copy link
Member

@daviddossett I think it is not just the V2 themes. The above screenshot is from GitHub theme. I also see the same in the custom theme I am using - davidbwaters.macos-modern-theme . Not sure if we would like to expect these themes to adopt their color tokens. I am fine to introduce background color for the profile indicator but using the badge background might be bit aggressive. How about introducing a new background color token for profile indicator that has a good deafult that goes well with some popular themes?

@daviddossett
Copy link
Contributor Author

You're one step ahead of me! I was just proposing that as an alternative to @esonnino in our call just now. That sounds like a good idea.

Definitely agreed that the colors should be muted compared to the loud badges we associate with some themes. I will create PR to add profile.badgeBackground and profile.badgeForeground and then update this PR to consume those once they are in.

@isidorn
Copy link
Contributor

isidorn commented Feb 22, 2023

@daviddossett thanks for tackling our concerns. A new colour sounds like the right approach here.

@daviddossett
Copy link
Contributor Author

  • Added profile.badgeBackground and profile.badgeForeground
  • Consumed new colors in profile badge CSS
  • Defaults can now be overridden by other themes
CleanShot.2023-02-22.at.13.55.30.mp4

@sandy081 sandy081 self-requested a review February 23, 2023 10:10
@sandy081
Copy link
Member

Looks good. Thanks for the changes.

There was a compilation issue and fixed it.

@isidorn
Copy link
Contributor

isidorn commented Feb 23, 2023

I just tried it out and I think this is going in the right direction -> approved.

I opened these follow up issues:
#175218
#175219

Let me know what you think. Thanks!

@isidorn
Copy link
Contributor

isidorn commented Feb 23, 2023

@sandy081 seems like the tests failed for a flaky reason?

@sandy081
Copy link
Member

Yeah seems flaky. Re-run again.

@sandy081 sandy081 merged commit 1516c2b into main Feb 23, 2023
@sandy081 sandy081 deleted the ddossett/domestic-horse branch February 23, 2023 10:51
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants