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

Add @sourceFontFamily LESS variable and use instead of SourceCodePro font-family #10727

Merged
merged 2 commits into from Mar 17, 2015

Conversation

Projects
None yet
4 participants
@humphd
Contributor

humphd commented Mar 9, 2015

In my Brackets fork, I've been discussing ways to save on download size for the in-browser use case. @peterflynn and I discussed making it easier to turn off the use of the Source Code/Sans Pro web fonts in favour of system fonts (it ends up saving me about ~300K of gzipped download, which is great), see https://github.com/humphd/brackets/issues/78.

I've made a start at this, switching all explicit uses of these font-familys to variables. I'm unclear how much more I can do than this, since all of the other occurrences of these are in extension LESS/CSS files, and I don't think these have access to the variables in Brackets (correct me if I'm wrong and can change there too)?

Anything else I can do here? I'm not sure how to deal with the explicit use in extensions, especially when I rip out that font-family for the in-browser case.

cc @Pomax

@le717

This comment has been minimized.

Show comment
Hide comment
@le717

le717 Mar 9, 2015

Contributor

I was actually going to do this as part of #8985, but it's probably better to submit it separately so it doesn't take as long to merge. :)

Contributor

le717 commented Mar 9, 2015

I was actually going to do this as part of #8985, but it's probably better to submit it separately so it doesn't take as long to merge. :)

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Mar 17, 2015

Member

Sigh... shame there are all those separate references in core extensions.

@humphd I think for now you'll have to live with just having more diffs in your fork than you'd like... unless you feel like biting off the work of getting extensions' LESS to compile in the context of the Brackets core LESS (or at least with a variable declarations file that's shared between them). That would significantly simplify the dark/light theming too, so it's definitely worth doing... but it's also a lot more work than just adding a dozen extra 1-line diffs on your end :-)

The code changes here look ok to me though. Just one thought: what about the .code-font() mixin defined in brackets_theme_default.less? For completeness we should probably add a @sourceFontFamily-medium variable too, even though there's only that one reference for now.

Member

peterflynn commented Mar 17, 2015

Sigh... shame there are all those separate references in core extensions.

@humphd I think for now you'll have to live with just having more diffs in your fork than you'd like... unless you feel like biting off the work of getting extensions' LESS to compile in the context of the Brackets core LESS (or at least with a variable declarations file that's shared between them). That would significantly simplify the dark/light theming too, so it's definitely worth doing... but it's also a lot more work than just adding a dozen extra 1-line diffs on your end :-)

The code changes here look ok to me though. Just one thought: what about the .code-font() mixin defined in brackets_theme_default.less? For completeness we should probably add a @sourceFontFamily-medium variable too, even though there's only that one reference for now.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Mar 17, 2015

Member

(Link: the card RESEARCH: Theming improvements & maintainability discusses shared LESS vars briefly, but it covers a lot of other ground too)

Member

peterflynn commented Mar 17, 2015

(Link: the card RESEARCH: Theming improvements & maintainability discusses shared LESS vars briefly, but it covers a lot of other ground too)

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 17, 2015

Contributor

@peterflynn thanks for reviewing. I agree that it's not worth shaving the "extensions' LESS" yak just to do this change. I'll do another commit for the .code-font() mixin later today or tomorrow, and this should be good to go.

Contributor

humphd commented Mar 17, 2015

@peterflynn thanks for reviewing. I agree that it's not worth shaving the "extensions' LESS" yak just to do this change. I'll do another commit for the .code-font() mixin later today or tomorrow, and this should be good to go.

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 17, 2015

Contributor

@peterflynn: Updated, ready for another look.

Contributor

humphd commented Mar 17, 2015

@peterflynn: Updated, ready for another look.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Mar 17, 2015

Member

Looks good.

Member

peterflynn commented Mar 17, 2015

Looks good.

peterflynn added a commit that referenced this pull request Mar 17, 2015

Merge pull request #10727 from humphd/font-variables
Add `@sourceFontFamily` LESS variable and use instead of SourceCodePro font-family

@peterflynn peterflynn merged commit 1e1feb9 into adobe:master Mar 17, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 18, 2015

Contributor

Awesome, thanks for the help with this, and for landing it so quickly.

Contributor

humphd commented Mar 18, 2015

Awesome, thanks for the help with this, and for landing it so quickly.

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 18, 2015

Contributor

FYI, doing follow-up work with this on our branch, I found one more use of SoureCodePro-Medium, but it's in JS here https://github.com/adobe/brackets/blob/master/src/view/ViewCommandHandlers.js#L94. I don't think there's a nice way to include that one in what we did above, so I've just changed it manually in our fork.

Contributor

humphd commented Mar 18, 2015

FYI, doing follow-up work with this on our branch, I found one more use of SoureCodePro-Medium, but it's in JS here https://github.com/adobe/brackets/blob/master/src/view/ViewCommandHandlers.js#L94. I don't think there's a nice way to include that one in what we did above, so I've just changed it manually in our fork.

@Pomax

This comment has been minimized.

Show comment
Hide comment
@Pomax

Pomax Mar 18, 2015

Oh that's an interesting rule, too: it relies on two windows-specific fonts (which used to require installing asian languages, but I don't think that's true anymore?) without rules for what to load on OSX and Linux

Pomax commented Mar 18, 2015

Oh that's an interesting rule, too: it relies on two windows-specific fonts (which used to require installing asian languages, but I don't think that's true anymore?) without rules for what to load on OSX and Linux

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