Skip to content

Switch from Stylus to CSS #636

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

Closed
wants to merge 7 commits into from
Closed

Conversation

mattico
Copy link
Contributor

@mattico mattico commented Mar 5, 2018

This is a big change in direction, so it deserves some consensus. I'm pretty happy with the end result, though, so I decided to just submit a PR.

Moving from Stylus has been discussed before, but to summarize:

  • Removes a build step and several dependenices.
  • Removes the confusion for users and contributors who don't notice that book.css isn't the real source for styling, or who don't understand Stylus syntax.
  • Makes it easy for users to write themes in CSS.

That last point is really cool because by just using CSS+variables, it's really easy to add custom theme support. The only change we'd need to make is add a config option for themes (just to make it easy to add the theme to the theme menu), and then include their CSS file which defines the same variables that the other themes do.

Here's what this PR does:

  1. Convert Stylus to CSS (manually).
  2. Remove Stylus supporting code and documentation.
  3. Merge the many CSS files into 4 files.
    Having lots of small files is annoying to deal with in our current theme implementation (fixing that is
    another discussion), and it's bad for page load times. While we could use a build step to concatenate
    it's much simpler to not, and we don't actually have enough CSS to need many files. We could easily
    just have one stylesheet, but I liked having a little bit of organization.

Closes #616.

@mattico mattico changed the title Remove stylus Switch from Stylus to CSS Mar 5, 2018
@Michael-F-Bryan
Copy link
Contributor

The only change we'd need to make is add a config option for themes (just to make it easy to add the theme to the theme menu), and then include their CSS file which defines the same variables that the other themes do.

I doubt this will be very hard. We need to figure out what themes are available anyway (probably via some additional-themes key in book.toml) in order to copy them to the output directory, so we can easily generate the themes selector from that.

@mattico
Copy link
Contributor Author

mattico commented Mar 7, 2018

Looks like I pushed e300d48 to the wrong branch... but it's fine being here. There aren't any changes since elasticlunr-rs 0.2.2, I'm just following the semver guide:

If your software is being used in production, it should probably already be 1.0.0.

@mattico
Copy link
Contributor Author

mattico commented Mar 7, 2018

I looked into implementing theme support with this. It gets a bit more complicated since themes will want to be able to set the theme used by highlight.js and ace and they'll probably want to be able to add their own highlight/ace themes. That is a solvable problem but it starts to get really hacky. I think we should postpone that idea until we figure out what to do about themes in general.

@mattico
Copy link
Contributor Author

mattico commented Mar 10, 2018

I should cc @sorin-davidoi @azerupi @budziq for your opinions.

@Michael-F-Bryan
Copy link
Contributor

I think we should postpone that idea until we figure out what to do about themes in general.

I agree. There are definitely ways we can hack in setting the theme used by highlight.js and the ace editor but they'll most probably be just that... hacks.

Either way, that probably comes under the larger umbrella of defining some sort of public interface so people can tweak themes. It's kinda related, but I'd say that issue is orthogonal to whether we use CSS or Stylus or whatever.

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.

Discussion: Should we migrate from Stylus, and to what?
2 participants