firefox-devtools / ux

Firefox DevTools UX Community
Mozilla Public License 2.0
103 stars 21 forks source link

Improve color of pseudo-classes #13

Closed violasong closed 5 years ago

violasong commented 5 years ago

This reddish orange looks like a warning and doesn't fit the theme in light or dark mode. Pick out some new colors (ideally colors that are already being used somewhere in DevTools).

image
fvsch commented 5 years ago

I think it might be meant to match the orange circle added in the Inspector gutter when forcing a :hover, :focus or :active state on a node. It might be more red than orange for better contrast.

So if a different color is picked, it should be applied to both elements. Note that it's also possible to use two different colors (e.g. 2 shades of the same Photon color) for the light and dark themes.

Resources:

violasong commented 5 years ago

Good point, thanks Florens!

chris-at-jobber commented 5 years ago

I'll take on this one!

fvsch commented 5 years ago

@chris-at-jobber Are you still up for working on this issue?

One way to proceed is to identify a Photon color that is not already overused in the Inspector and could work for pseudo-classes. It doesn't have to have an obvious meaning (I don't think that "conditional state" has a color, but I could be wrong.) Orange should be out of the equation because it's reserved for the Firefox brand, but any other could work.

We should probably use the same Photon base color (Photon Blue, Photon Green, or whichever you pick) for both the dark and light themes and for both the selector text and the disc indicator in the markup view. But we can use different values of the same base color (e.g. Blue 50, Blue 70, etc.).

A simple-ish way to mock up changes is to open DevTools on a page, then open the Browser Toolbox (usage guide here) to inspect DevTools and tweak its CSS color values. When you're happy with the result, you can make a screenshot and share it here (ideally noting the Photon color values used).

chris-at-jobber commented 5 years ago

@fvsch yes sorry! This was some of the initial work I had put together, I'll try and do something productive with this shortly.

fvsch commented 5 years ago

That's great, thanks!

akshithashetty commented 5 years ago

Hi, Is this issue available ? I would like to start work on this one.

chris-at-jobber commented 5 years ago

Sorry yes feel free! I have not been able to put the time towards it that I had been hoping.

On Mon, Feb 11, 2019 at 4:26 AM akshithashetty notifications@github.com wrote:

Hi, Is this issue available ? I would like to start work on this one.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/devtools-html/ux/issues/13#issuecomment-462295323, or mute the thread https://github.com/notifications/unsubscribe-auth/Al3ZRV8fffNLteKPRIgjytsLmizjUNiLks5vMVNvgaJpZM4WGygu .

--

CHRIS MURRAY Senior Product Designer

[image: Jobber Logo] chris.m@getjobber.com 10520 Jasper Ave, Edmonton, AB, T5J 1Z7 www.getjobber.com http://getjobber.com/

akshithashetty commented 5 years ago

Thanks 👍

akshithashetty commented 5 years ago

Hi, This is the initial set of suggestions I have, As the inspector extensively has shades of blue and magenta, I have avoided using blue, so that the pseudo class text is highlighted. Also, as discussed earlier orange is reserved for firefox branding so from the remaining photon colors, I have implemented green and yellow and taken screenshots of the same. It would be great help if I could get some further direction on this. Thanks.

slice 1 slice 2 1

fvsch commented 5 years ago

Let's see what @violasong says.

I think I like the yellow option better, especially since we're already using green for HTML comments and we can have of lot of those in the markup view (like in your example). The yellow shades draw attention, but don't look like a "warning" to me here, so I would be okay using them.

We can also use two different shades of the same Photon color for the selector text and the disc marker in markup view, if that looks better. For example, in the light theme Yellow 70 works well for the selector text, but we could use Yellow 60 for the disc if that looks better.

violasong commented 5 years ago

This is great @akshithashetty! I agree with Florens that the yellow seems better.

I do think the discs would look more integrated if the light theme one was 1-2 steps lighter and the dark theme one was 1-2 steps darker. Even doing 1-step change for the text might be nice (if the contrast is still high enough)

akshithashetty commented 5 years ago

Thanks @violasong @fvsch for the feedback. That helps me come up the following revision. Let me know what you think about this - slice 3

fvsch commented 5 years ago

Looks pretty good!

I'm wondering if we should remove the bold font-weight on the pseudo-selector, too. Those colors are contrasted enough and maybe don't need the extra weight.

violasong commented 5 years ago

Yes, looks good, and I agree that the bold might not be necessary.

The discs in the dark mode look surprisingly orange as opposed to the #d7b600 dark yellow - is it just a screenshot difference?

akshithashetty commented 5 years ago

Sorry, I think the change was not applied at the moment I took the screenshot, I have rectified it and also changed the font weight attached the following screenshot. Let me know what you have to say about this one - slice 4

violasong commented 5 years ago

Ah, I see! This new screenshot looks fantastic!

On Tue, Feb 26, 2019 at 2:36 PM akshithashetty notifications@github.com wrote:

Sorry, I think the change was not applied at the moment I took the screenshot, I have rectified it and also changed the font weight attached the following screenshot. Let me know what you have to say about this one - [image: slice 4] https://user-images.githubusercontent.com/31488143/53451516-f25ed880-3a44-11e9-8f0f-1762db5009b7.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/firefox-devtools/ux/issues/13#issuecomment-467643399, or mute the thread https://github.com/notifications/unsubscribe-auth/AADauhTtwm57keJIrabJufTsrr_jTM_8ks5vRbcFgaJpZM4WGygu .

akshithashetty commented 5 years ago

Glad it works! So should I go ahead making the changes to code?

fvsch commented 5 years ago

@akshithashetty That would be great. Changes should happen in the mozilla-central repository, so we need a Bugzilla bug to work on. There was an existing one with a very close focus, so I added some details in it: https://bugzilla.mozilla.org/show_bug.cgi?id=1200686#c2

If you've never worked on mozilla-central before, fair warning, it takes a little while to set up. We have a guide here. I would recommend opting in to artifact builds before running ./mach build, this will make the build much faster.

akshithashetty commented 5 years ago

Thanks @fvsch. That helps! I have solved a few bugs on mozilla-central and have the setup in place. I will try to fix it and generate a review request in phabricator. Will surely ask for info incase there is something unclear.

fvsch commented 5 years ago

This should land in Nightly tonight or tomorrow. Thanks a bunch @akshithashetty for your color tests and the CSS patch!

Copying the text version of the design spec here, if we want to put it in a design document:

violasong commented 5 years ago

I just noticed this in Nightly and it looks great! Great work @akshithashetty, and thanks for mentoring, @fvsch!

Btw, Akshitha — if you have a twitter account, I would love to mention you in a @firefoxdevtools tweet about this nice example of hybrid UX/CSS work :).

akshithashetty commented 5 years ago

Glad to see the changes in Nightly. Thanks @violasong @fvsch for mentoring this bug. It was great learning and fun. I have a twitter account - AkshithaShetty8. I am looking forward to the @firefoxdevtools tweet about this issue. Thanks again !