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

Themes in Core v1 #8302

Merged
merged 109 commits into from
Jul 14, 2014
Merged

Themes in Core v1 #8302

merged 109 commits into from
Jul 14, 2014

Conversation

MiguelCastillo
Copy link
Contributor

Fresh branch from a clean master

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.
@MiguelCastillo
Copy link
Contributor Author

@dangoor Awesome! I will go through those follow up items and work my way through them.

@MiguelCastillo
Copy link
Contributor Author

@dangoor We should add in the list the migration to the fonts api

@ryanstewart
Copy link
Contributor

This is awesome! Can't wait to see this in the release. Major props @MiguelCastillo! 💥

@dangoor
Copy link
Contributor

dangoor commented Jul 14, 2014

@MiguelCastillo We could either add a new issue for that or just modify the existing pull request (#8305)

@larz0
Copy link
Member

larz0 commented Jul 14, 2014

I'll test this today. Great timing!

* @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
* ThemeManager
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing id paramenter

@TomMalbran
Copy link
Contributor

@MiguelCastillo I went through all the JSDocs and gave you some feedback.

@MiguelCastillo
Copy link
Contributor Author

@TomMalbran Thanks for your feedback! Definitely helpful. I wasn't sure how to do the one for array of themes. :)

@MiguelCastillo
Copy link
Contributor Author

@TomMalbran I will make these adjustments part of the PR for the font api if that's ok

@TomMalbran
Copy link
Contributor

Sure. But might be nice to have good JSDocs before the next version so that the API Docs are accurate.

@MiguelCastillo
Copy link
Contributor Author

You don't think we will be landing this one soon?

@MiguelCastillo
Copy link
Contributor Author

Sorry, not this one... Font api PR

@TomMalbran
Copy link
Contributor

I don't know. I hope it does.

@MiguelCastillo
Copy link
Contributor Author

Hmmm, that could a problem... There is cleanup in Themes that's pending on the font API. And this issue #8381

@peterflynn
Copy link
Member

Yeah, it's probably safe to assume it'll have to land in this sprint since it's fixing issues that we don't want to ship with...

@MiguelCastillo
Copy link
Contributor Author

@peterflynn Awesome!

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

Successfully merging this pull request may close these issues.

8 participants