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

[Themes] Should we allow multiple themes per extension? #8423

Closed
peterflynn opened this issue Jul 16, 2014 · 6 comments
Closed

[Themes] Should we allow multiple themes per extension? #8423

peterflynn opened this issue Jul 16, 2014 · 6 comments

Comments

@peterflynn
Copy link
Member

This would be useful for providing theme "packs." A number of themes come in light+dark pairs, so this is probably a fairly common need. It would also be useful if any of the existing theming extensions want to convert over to just being a bundle of popular themes that plug into the core theming feature -- more than just the convenience of downloading all their old themes at once, it would let them actually update in place, removing their old menu items and morphing directly into a theme bundle.

I know we eventually want to allow bundling any set of extensions together into a single-install "pack", but that's a long way down the road still. Themes are something we could address in a much more targeted, simple way.

E.g. instead of "theme": "<somefile>.less" in package.json, we could do this:

"themes": {
    "Theme 1": "theme1.less",
    "Theme 2": "theme2.less",
    ...or maybe:
    "Theme 3": {
        file: "theme3.less",
        dark: true,
        <any other special settings needed>
    }
}
@MiguelCastillo
Copy link
Contributor

@peterflynn I love the idea, and that's exactly why I did https://github.com/MiguelCastillo/brackets/blob/master/src/view/ThemeManager.js#L149. So that you can have a main.less which does

@import "theme1.less";
@import "theme2.less";

I have also added this https://github.com/MiguelCastillo/brackets/blob/master/src/view/ThemeManager.js#L450. This allows you to load a particular directory with all kinds of themes. I myself will converting my themes extension to plug into the theme manager so that I can load CodeMirror themes and others I already have.

Either way, adding what you are asking isn't really too difficult at all, since the API in ThemeManager can easily be enhanced to do that.

I can put together a branch that does that if you want to try it. The config processing would just happen right here https://github.com/MiguelCastillo/brackets/blob/master/src/view/ThemeManager.js#L338.

@MiguelCastillo
Copy link
Contributor

@peterflynn Alright, so the imports won't do the multipack thing... Once themes settles down a bit, I will add logic for this if you are interested in trying it.

@dangoor
Copy link
Contributor

dangoor commented Jul 17, 2014

@MiguelCastillo I think we need to change the package.json format now in order to give the theme a chance to express whether it is "light" or "dark" so that the rest of the Brackets UI can adapt. In #8415, I suggest that editor themes may also be able to suggest a few colors outside of the editor area. We'd need a place to put that extra metadata.

I'm not actually particular about being able to bundle themes into a single extension. It is true that some themes come in light/dark pairs, but I'm not sure if users of those themes necessarily care to have both.

What I do think is that the metadata needs to change to either what Peter suggests here or:

"theme": {
    "file": "theme.less",
    "dark": true
}

I don't think we want to go down the path of having package.json support theme as a string or object or themes as an object.

By the way, Atom uses the theme key in package.json to denote whether a theme is a UI theme or syntax theme. They use filename conventions to know where to look for files (they automatically load the stylesheets that are in the stylesheets directory). I do like the idea of an editor theme giving a clue about how the UI should be styled, which is something that has no place in Atom's scheme.

@MiguelCastillo
Copy link
Contributor

Just closing the loop on this issue. I have pushed a change to support a theme object for single themes. No theme packs support, sorry @peterflynn :/

#8447

@dangoor
Copy link
Contributor

dangoor commented Jul 21, 2014

Oh yeah, I meant to add for posterity here a bit of discussion that @MiguelCastillo and I had on IRC. My thought was that people probably don't use more than 1 or 2 themes generally. If someone made a 10 theme pack, then we'd probably start getting comments from people who want a UI to let them hide 8 of those 10 themes from the theme manager (so they don't appear in the selection UI).

Also, while theme authors sometimes create both light and dark variants of a theme, I'm not sure if the users use both.

I think it makes more sense to make themes distinct extensions and have generic extension bundles be a solution if there are people who want multiple themes.

I also think the pressing concern is making themes easy to browse before installation.

@dangoor dangoor changed the title Should we allow multiple themes per extension? [Themes] Should we allow multiple themes per extension? Jul 21, 2014
@MiguelCastillo
Copy link
Contributor

This issue has already been addressed. We basically decided to only allow one theme per extension.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants