Fonts api with Themes integrated #8492
Conversation
Truncate extension descriptions
| Object.keys(newSettings).forEach(function (setting) { | ||
| prefs.set(setting, newSettings[setting]); | ||
| if (prefs.hasOwnProperty(setting)) { |
There was a problem hiding this comment.
This shouldn't work. Preferences aren't properties of the PrefixedPreferencesSystem which is what prefs is.
There was a problem hiding this comment.
What was I thinking!? Sorry man, I will patch that up and submit a PR. Thanks for merging this. Its a real nightmare to keep themes changes and fonts synced!
There was a problem hiding this comment.
Oh sorry, i thought this was merged and that's why it was closed. Misread
| $style.html(cssRule + "{ " + styleStr + " }"); | ||
|
|
||
| // Let's make sure we remove the already existing item from the DOM. | ||
| _removeDynamicProperty(propertyID); |
There was a problem hiding this comment.
Nit: It seems to me that these nodes will always exist (after they're initially created), so we might as well just add them once and change the contents, rather than adding/removing them.
That's a nit, though, because I don't think there would be any functional difference and any performance difference is irrelevant in this context... I wouldn't block landing for that.
There was a problem hiding this comment.
Oh yeah man, we can definitely reuse the same nodes. I tried to keep as much of what was there as possible. I changed quite a bit of code as it is.
|
@MiguelCastillo You started saying something last night about how the line height setting may need to go, but I don't think you finished that thought because of the scrollbar discussion. What was your concern about line height? |
|
I was just about to bring that up again... :) So I think having line height is really useful, and that's why I have added that in. But it seems a bit fragile because I am changing spans to be inline-blocks so that spans get a height that takes into account padding and what not. We will just need to be much more careful with CodeMirror plugins we introduce because if they use spans to decorate the DOM then that's an immediate candidate for introducing issues. |
|
I agree that line height is useful. It's a matter of personal taste and can make a big difference in readability. |
|
It's really nice that I can finally use Comic Sans MS as my coding font. |
|
Yup, exactly why I don't want to remove it. I have it set to 1.4, so I personally think it is needed. Cool, let's leave it and we can deal with any hurdles when they come up :) |
|
Oh that's right. Font family is one of the reasons I added the restore settings button. So, I may want to play with a font, and maybe I want to just go back because what I added really sucks... "Restore me baby" is what the button should say :) This would be alleviated by applying settings right away without having to dismiss the dialog, and still be able to cancel out of the whole thing. |
| @@ -347,6 +551,10 @@ define(function (require, exports, module) { | |||
| CommandManager.register(Strings.CMD_THEMES, Commands.CMD_THEMES_OPEN_SETTINGS, _handleThemeSettings); | |||
|
|
|||
| PreferencesManager.convertPreferences(module, {"fontSizeAdjustment": "user"}, true, _convertToNewViewState); | |||
|
|
|||
| prefs.definePreference("fontSize", "string", DEFAULT_FONT_SIZE + "px"); | |||
There was a problem hiding this comment.
As a general rule, you should listen for preference changes. It's not as obvious in this case, because we have actual UI for these preferences. But, I just deleted some of the settings from my preferences file and the changes don't take effect until I reload because there's no listener here.
There was a problem hiding this comment.
Yeah man, I had that before... I removed it because for some odd reason the listener was never getting called. I guess I should have asked :/
There was a problem hiding this comment.
I generally just do something like this:
prefs.definePreference("fontSize", "string", DEFAULT_FONT_SIZE + "px").on("change", function () {
// do stuff
});
|
This change gave me the idea that the I realized that there's not a lot of value in changing |
|
Merging |
Fonts api with Themes integrated
|
Thank you Kevin! Should we log an issue for prefs changes? |
|
@MiguelCastillo I just filed #8499 for the font and theme prefs listeners. |
|
@MiguelCastillo Good job. But the highlight matches are using a bottom border, so now each time one is selected, the line height is 2px taller. |
Fixes:
#8425
#8381
References:
#8305