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

Make font settings react to pref changes #11190

Merged
merged 2 commits into from Jun 1, 2015

Conversation

Projects
None yet
4 participants
@MarcelGerber
Contributor

MarcelGerber commented May 31, 2015

This should go with, or before, #11130.

Show outdated Hide outdated src/view/ViewCommandHandlers.js
@@ -49,6 +49,10 @@ define(function (require, exports, module) {
var prefs = PreferencesManager.getExtensionPrefs("fonts");
// These variables contain the preference values. They are used to no-op in case nothing changed
// and updated *after* the call to setFontSize/setFontFamily.
var fontSizeValue, fontFamilyValue;

This comment has been minimized.

@MiguelCastillo

MiguelCastillo May 31, 2015

Contributor

Let's add proper jsdoc to these two variables and mark them as private.

@MiguelCastillo

MiguelCastillo May 31, 2015

Contributor

Let's add proper jsdoc to these two variables and mark them as private.

@MiguelCastillo

This comment has been minimized.

Show comment
Hide comment
@MiguelCastillo

MiguelCastillo May 31, 2015

Contributor

@MarcelGerber changes look good. I am not even sure why I didn't add this with the initial PR, so thanks :)

Contributor

MiguelCastillo commented May 31, 2015

@MarcelGerber changes look good. I am not even sure why I didn't add this with the initial PR, so thanks :)

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Jun 1, 2015

Contributor

@MarcelGerber thanks for this change! I just have a s comment. Can we change the variable name to something like currFontSizeValue. I guess it would improve the readability of the code. Otherwise LGTM.

Contributor

nethip commented Jun 1, 2015

@MarcelGerber thanks for this change! I just have a s comment. Can we change the variable name to something like currFontSizeValue. I guess it would improve the readability of the code. Otherwise LGTM.

@abose abose added this to the Release 1.4 milestone Jun 1, 2015

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Jun 1, 2015

Contributor

Changes pushed.

Contributor

MarcelGerber commented Jun 1, 2015

Changes pushed.

@MiguelCastillo

This comment has been minimized.

Show comment
Hide comment
@MiguelCastillo

MiguelCastillo Jun 1, 2015

Contributor

looks good, works well. merging in.

Contributor

MiguelCastillo commented Jun 1, 2015

looks good, works well. merging in.

MiguelCastillo added a commit that referenced this pull request Jun 1, 2015

Merge pull request #11190 from adobe/marcel/fonts-react-prefs
Make font settings react to pref changes

@MiguelCastillo MiguelCastillo merged commit 83b632d into master Jun 1, 2015

1 check passed

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

@abose abose deleted the marcel/fonts-react-prefs branch Jun 1, 2015

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