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

Extend theming guide with "files" section #252

Merged
merged 6 commits into from
Oct 26, 2021
Merged

Extend theming guide with "files" section #252

merged 6 commits into from
Oct 26, 2021

Conversation

deejayy
Copy link
Contributor

@deejayy deejayy commented Oct 24, 2021

While I was making a theme, I had to debug and analyze the source code to discover the ability to distribute files in a theme. This PR is to make it straightforward for others. Feel free to rephrase or ask me to do the same (suggestions welcome, I'm not native).

Copy link
Member

@brunnre8 brunnre8 left a comment

Choose a reason for hiding this comment

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

Thanks a bunch, this is definitely needed.
There's some things that could be improved in my opinion, what do you think?

},
```

After installing the theme, these files will be available under `/plugins/<your-theme>/<file-name>`.
Copy link
Member

Choose a reason for hiding this comment

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

This is not true unless I'm missing something.
/ in this nomenclature applies to the http request root, the path from that root is /packages/:package/:filename

Below in your example you actually give the proper path.

Copy link
Member

Choose a reason for hiding this comment

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

The folder path as is on disc doesn't really matter for the reader, as they can't control it directly anyhow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I was bound to the web context, what do you think about

After installing the theme, these files will be available under the web folder /plugins/...

Copy link
Member

Choose a reason for hiding this comment

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

Expect it's not.
TL forces a subfolder for your package, if you have a file array containing foo.gif the http path to is

/packages/<package name>/foo.gif

The reader only cares about that, not sure why you want to mention a plugins dir?
Where do you get that from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it's packages not plugins, sorry, just a typo!

@@ -34,6 +34,29 @@ This will create a `package.json` file that you must edit as such:

Although it is not required, we strongly recommend you also fill in the `"homepage"`, `"repository"`, and `"bugs"` sections.

Optionally, you can distribute other files along with the theme stylesheet as follows:
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 you should give it a header so that this is its own section.
Or shove it into a bullet point, not sure which makes more sense. I tend to favor the section as it's easy to tell apart from the minimum setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the section idea!

```css
@font-face {
font-family: "Alternative Font Name";
src: url(/packages/thelounge-theme-your-theme/alternative-font.woff2) format("woff2");
Copy link
Member

Choose a reason for hiding this comment

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

url(/packages/<package name>/alternative-font.woff2) maybe? The repeated "theme" in that path makes it rather hard to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I considered is the general theme naming convention (maybe unwritten) that the themes always start with thelounge-theme- prefix. Other thing is to keep the example code free of template like parts, e.g. to be a valid CSS. (otherwise the font name, file name should also be templated - in my opinion it'd make the example awkward). What do you think?

Copy link
Member

@brunnre8 brunnre8 Oct 25, 2021

Choose a reason for hiding this comment

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

templates are fine, it shows the reader what to substitute.
We don't need to overdo it, the file part is obvious, especially if they read the full guide.
However you never actually show the package name (as you only show the TL section of package.json) so the reader has no clue where suddenly that thing is coming from.

I'd simply write it as I proposed above, being valid css isn't important as you are talking to a human, not a browser.
It just needs to look like it and it still does that.

Copy link
Member

Choose a reason for hiding this comment

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

If you do insist on an example over a semi templated form, at least make it something sane, thelounge-theme-example, not -your-theme

-your-theme is basically the same as templating, just with much more noise and I certainly stumbled over it while reading the diff, had to backtrack and validate that we really are stuttering and that it wasn't me messing up the reading part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally makes sense, thanks!

@aab12345
Copy link
Contributor

aab12345 commented Oct 25, 2021

@deejayy

Change line 51 and line 56 to something like what brunnre8 mentioned

/packages/<package name>/alternative-font.woff2

That should do it.

src: url(/packages/<package name>/alternative-font.woff2) format("woff2");
}
```

Copy link
Member

Choose a reason for hiding this comment

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

(Can't comment on the proper line, GH only let's me comment lines you edited apparently)

Move the

For a comprehensive example, refer to the [package.json file of thelounge-theme-solarized](https://github.com/thelounge/thelounge-theme-solarized/blob/master/package.json

part back up to the main section, else it looks good. Thanks!

Copy link
Member

@brunnre8 brunnre8 left a comment

Choose a reason for hiding this comment

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

Neat, looks good to me. Thanks for the effort and persistence.

@MaxLeiter heads up: this needs a squash merge if you are fine with it.

@MaxLeiter MaxLeiter merged commit 94b8c8d into thelounge:master Oct 26, 2021
@MaxLeiter
Copy link
Member

Thanks @brunnre8 and @deejayy 🙏

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

Successfully merging this pull request may close these issues.

None yet

4 participants