-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
@@ -0,0 +1,24 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably just go in Settings.js
All used strings should be localized. And btw, Travis failed (because of missing JSLint comments). |
return PreferencesManager.ready.then(function() { | ||
if ( Settings.getValue("paths") === (void 0) ) { | ||
Settings.setValue("paths", DefaultSettings.paths); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess prefs.definePreference()
should be used here to define default settings
Yeah, there are a few items in my list for code cleanup and integrate with what brackets already has. I think that by tomorrow or friday, code will be more inline with what we all expect :) |
Love how quickly you guys are providing feedback! Its awesome |
For some added context for those tuning in to this pull request right now: I asked Miguel if he could convert his extension into a pull request against core as something that we could begin discussing and also so that it was possible for it to load themes as extensions, which is not possible in an extension today. I added (REVIEW ONLY) because there's definitely more coming before this feature lands. |
@@ -109,6 +109,7 @@ define(function (require, exports, module) { | |||
exports.SORT_WORKINGSET_BY_NAME = "view.sortWorkingSetByName"; // WorkingSetSort.js _handleSortWorkingSetByName() | |||
exports.SORT_WORKINGSET_BY_TYPE = "view.sortWorkingSetByType"; // WorkingSetSort.js _handleSortWorkingSetByType() | |||
exports.SORT_WORKINGSET_AUTO = "view.sortWorkingSetAuto"; // WorkingSetSort.js _handleAutomaticSort() | |||
exports.THEMES = "view.themes"; // themes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stay in sync with the detailed comments of the other entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably also use CMD_THEMES
after the change done with the Find commands
This works by checking if the “theme” field exists in the package.json to determine if the extension ought to be treated as a theme or not.
So that we can preemptively figure out if the extension is a theme or a regular extension.
Alright, I think we are at a point where feedback would be great. Themes code is fully integrated into Brackets. I created a theme that is already in the Extension Registry. The theme has to have a main.js or the registry wouldn't allow me to load it. So, we might need to adjust something there. This is the git link for the theme also I am going to evaluate inline editor and mixin support. #4850 |
/** | ||
* @private | ||
* Verifies if an extension is a theme based on the presence of the field "theme" | ||
* in the package.json. If it is a theme, then the theme file is just loaded by the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 2 spaces
@MiguelCastillo give me 5mins, I got disconnected from IRC for pasting too much console.log. |
@MiguelCastillo it seems like "Restore font size" won't work. Just a heads up. |
Umh sorry for my intrusion ^_^" I've cloned this PR but when I apply a theme using "View > Themes" nothing happens... Is it normal? |
@FezVrasta Ah! No intrusion at all! We want your feedback. You don't get a dialog at all? |
I can open the window where I can choose the theme but when I click save the dialog is closed without any change of the theme. |
Would you like to jump in IRC so that I can help you, help me fix the issue? |
Sure, I'm there. This is the log I get:
|
function loadCurrentThemes() { | ||
return $.when(undefined, _.map(getCurrentThemes(), function (theme) { | ||
if (!theme) { | ||
return $.Deferred().reject("Theme not found"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should actually be:
return new $.Deferred().reject("Theme not found").promise();
Unfortunately, that error doesn't give a lot of information (what theme wasn't found?). It appears that information is not available in this context. It's not clear to me why getCurrentThemes() would even return an empty value.
My suggestions for tests would be:
|
…This cleaned up the rejected promises being returned. Fixed issue in loadCurrentTheme incorrect $.when call. Cleaned up a few other small issues
Conflicts: src/nls/root/strings.js src/styles/brackets_codemirror_override.less
…er for the settings UI
Closing this PR in favor of #8302. |
@FezVrasta Hey! I fixed the issue you are running into in Windows... If you want to give latest another try, that would be really nice :) https://github.com/MiguelCastillo/brackets branch themes-v1. |
Yes seems fixed now :) |
Initial commit. A few items remain to be done, but themes are now fully functional as a core component so you can start testing this out.
https://github.com/adobe/brackets/wiki/Themes