Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Change font weight of menu items to 400 instead of 200, improves readability #9829

Merged
merged 1 commit into from
Jan 29, 2015
Merged

Change font weight of menu items to 400 instead of 200, improves readability #9829

merged 1 commit into from
Jan 29, 2015

Conversation

Mark-Simulacrum
Copy link
Contributor

Fixes #9828.

Things to consider:

  • May not be the only place in the UI that this error happens.
  • Should the font weight used (400) be put into a LESS variable? If so, how should that variable be named?
    • We already have @font-weight-semibold: 500 and @font-weight-light: 200.

@le717
Copy link
Contributor

le717 commented Nov 6, 2014

FWIW. I'd call the LESS variable (if needed) @font-weight-normal. :)

@nmabhinandan
Copy link

This definitely fixes the issue.

peterflynn
peterflynn reviewed Nov 7, 2014
View reviewed changes
src/styles/brackets_patterns_override.less
@@ -328,7 +328,7 @@ a:focus {
padding: @menubar-top-padding @menubar-h-padding @menubar-bottom-padding;
border: 1px solid rgba(0, 0, 0, 0); // transparent border just to hold the layout so it doesn't shift on hover
color: fadeout(@bc-menu-text, 25%);
font-weight: @font-weight-light;
font-weight: 400;
Copy link
Member

Choose a reason for hiding this comment

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

This should be font-weight: normal which is exactly the same as 400. (See https://developer.mozilla.org/en-US/docs/Web/CSS/font-weight#Values)

@peterflynn
Copy link
Member

Assigning to @placegraphichere for visual review. (Note: to test this on a non-Linux computer, modify Global.js so that global.brackets.nativeMenus is always set to false). Changing the font-weight seems reasonable to me, but we could also consider changing the color for a similar effect.

RaymondLim
RaymondLim reviewed Nov 14, 2014
View reviewed changes
src/styles/brackets_patterns_override.less
@@ -328,7 +328,7 @@ a:focus {
padding: @menubar-top-padding @menubar-h-padding @menubar-bottom-padding;
border: 1px solid rgba(0, 0, 0, 0); // transparent border just to hold the layout so it doesn't shift on hover
color: fadeout(@bc-menu-text, 25%);
font-weight: @font-weight-light;
font-weight: normal;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since normal is the default value, I wonder we can just remove this property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: Just removed the line, seems that the setting is not needed; not set to another value anywhere else.

@Mark-Simulacrum
Copy link
Contributor Author

Should I squash the commits? Feels odd to me to have 3 commits for 1 tiny change.

@ingorichter
Copy link
Contributor

@Mark-Simulacrum Do you still want to squash the commits?
@placegraphichere Did you get a chance to look at this PR?

@Mark-Simulacrum
Copy link
Contributor Author

@ingorichter Squashed commits, thank you for the reminder!

@ingorichter
Copy link
Contributor

LGTM
Before:
screenshot 2014-11-21 16 22 37
After:
screenshot 2014-11-21 16 23 05

@ingorichter
Copy link
Contributor

@placegraphichere Do you want to have another look at the visual representation or does the change look good to you and I can merge it?

@nmabhinandan
Copy link

The problem was primarily in the dark theme running in Ubunut x64 OS. #9828

@le717
Copy link
Contributor

le717 commented Jan 16, 2015

@ingorichter Do you want to get this in 1.2? While I'm not a Linux user, this does fix a visibility issue (which can effect usability), and know it if affected me I'd want this in as soon as possible.

@nmabhinandan
Copy link

Yes please.

@ingorichter
Copy link
Contributor

With lighter font
linux menu with light font

With heavier font
linux menu with heavier font

ingorichter added a commit that referenced this pull request Jan 29, 2015
@ingorichter
Merge pull request #9829 from Mark-Simulacrum/bolder-menu-font
f962b99 
Change font weight of menu items to 400 instead of 200, improves readability
@ingorichter ingorichter merged commit f962b99 into adobe:master Jan 29, 2015
@Mark-Simulacrum Mark-Simulacrum deleted the bolder-menu-font branch January 31, 2015 14:36
@busykai busykai mentioned this pull request May 24, 2015
Closed
This was referenced Aug 30, 2021
Open
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
PR Triage Complete
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fonts of titlebar menu items aren't rendering properly in Ubuntu.
6 participants
@Mark-Simulacrum @le717 @nmabhinandan @peterflynn @ingorichter @RaymondLim