Skip to content
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

Add themes docs #69

Merged
merged 2 commits into from
Oct 5, 2017
Merged

Add themes docs #69

merged 2 commits into from
Oct 5, 2017

Conversation

AlMcKinlay
Copy link
Member

No description provided.

"version": "1.0.0",
"description": "A theme for The Lounge",
"main": "package.json",
"lounge": {
Copy link
Member

Choose a reason for hiding this comment

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

add display name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

"name": "lounge-theme-custom", // Replace 'custom' with your theme's name
"version": "1.0.0",
"description": "A theme for The Lounge",
"main": "package.json",
Copy link
Member

Choose a reason for hiding this comment

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

Document why we do this, and explain that an actual JS file can be used to export.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did this by adding another section below, let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

Does this, referencing a JS file that exports an object, really work?
I believe this does, but it would get stopped by that first if I understand correctly.

It's fairly common for libraries to use a key in the package.json like Babel, but I don't remember seeing them asking to export package.json or an object in JS, do you have an idea of how they access it?
Maybe I'm looking at this wrong, sorry if that's the case 😅

@AlMcKinlay
Copy link
Member Author

Updated this with the correct install instructions and stuff from @xPaw


To install theme `lounge-theme-custom`

* `npm install --prefix $LOUNGE_HOME/packages lounge-theme-custom`
Copy link
Member

@astorije astorije Sep 17, 2017

Choose a reason for hiding this comment

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

Maybe until lounge install is ready, it's worth adding export $LOUNGE_HOME=<config path of The Lounge> and mkdir -p $LOUNGE_HOME/packages (EDIT: just setting the env var is enough) before that line?
See thelounge/thelounge#1266 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to set-g as well, though I couldn't figure out the differences with/without when using --prefix...

Copy link
Member

@astorije astorije Sep 17, 2017

Choose a reason for hiding this comment

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

$ npm install --prefix $LOUNGE_HOME/packages lounge-theme-solarized
/Users/jeremie/.lounge/packages
└── [email protected]

npm WARN enoent ENOENT: no such file or directory, open '/Users/jeremie/.lounge/packages/package.json'
npm WARN packages No description
npm WARN packages No repository field.
npm WARN packages No README data
npm WARN packages No license field.

vs.

$ npm install --global --prefix $LOUNGE_HOME/packages lounge-theme-solarized
/Users/jeremie/.lounge/packages/lib
└── [email protected]

🤷‍♂️


EDIT: when intalling with --prefix and --global, themes can't be found, so whatevs. --prefix alone is good (@xPaw, reminder to remove the global flag from your lounge install command).

Copy link
Member Author

Choose a reason for hiding this comment

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

EDIT: when intalling with --prefix and --global, themes can't be found

That'll be because when installing with --global, they get put in $LOUNGE_HOME/packages/lib/node_modules rather than $LOUNGE_HOME/packages/node_modules.

Definitely possible to do, but we'll need a code change for that obviously.

Happy wither way.

Maybe until lounge install is ready, it's worth adding export $LOUNGE_HOME= and mkdir -p $LOUNGE_HOME/packages (EDIT: just setting the env var is enough) before that line?

Hmmm, I was assuming that we were just referring to $LOUNGE_HOME in the documentation as where they set their home to be, and they should replace the envar with that. I wasn't actually thinking people would actually use that. ¯\_(ツ)_/¯


```
{
"name": "lounge-theme-custom", // Replace 'custom' with your theme's name
Copy link
Member

Choose a reason for hiding this comment

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

Comments are not supported in package.json, so if people copy-paste as is, that could create confusion.

Also, I don't think we should document about license, issue tracker URL, etc. Just say that themes are normal npm packages, link to npm docs (like this one), and add about the lounge property and keywords.
I understand it's a kind thing to do though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmmm alright, how about it just talk about the exports, and mention that if you add "main: package.json" to your package.json, you can have the exports in that. Might be cleaner.

@astorije
Copy link
Member

astorije commented Oct 1, 2017

@YaManicKill, I took the liberty to add a commit with changes regarding the key (with regards to thelounge/thelounge#1583). Besides that, were there any other changes you wanted to make before we can merge this?

@astorije
Copy link
Member

astorije commented Oct 1, 2017

Oh, and I switch to using lounge install as well :)

@AlMcKinlay
Copy link
Member Author

Yeah, I think I'm happy with this, feel free to merge if you are happy as well @astorije

@astorije
Copy link
Member

astorije commented Oct 5, 2017

Good enough, we'll improve over time, and I'll make sure to give it a pass during doc rewrite. It's good to have it prior to that so it can be properly categorized, and so that nothing is forgotten.

Thanks for this @YaManicKill!

@astorije astorije merged commit 249f343 into master Oct 5, 2017
@astorije astorije deleted the themes branch October 5, 2017 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants