dwmkerr / crosswords-js

Tiny, lightweight crossword control for the web.
https://dwmkerr.github.io/crosswords-js/
MIT License
71 stars 27 forks source link

More css vars - take 2 #44

Closed mmkal closed 8 months ago

mmkal commented 8 months ago

Second attempt at #41

This doesn't go as far as dropping LESS completely (per @pvspain's comment), but it does reduce the dependency by switching over more LESS variables to CSS variables.

The rationale (summing up from the other PR): it makes customising styling easier for developers using this library - CSS variables can be set in userland CSS files which override defaults in crosswords-js, and they will be applied consistently everywhere (for example, changing the cell "highlight" colour and making sure it's in sync across clues, the grid and any other elements users might add; or adding a new screen-width breakpoint with a bigger or smaller cell size).

I've added some docs under ### Styling in the root readme. They could be more exhaustive but they will point developers using this library in the right direction, and I want to avoid docs churn.

Rationale for not ditching LESS right now:

  1. CSS variables can't be used in media queries but LESS variables can - and they're useful in making sure the screen-width queries are non-overlapping.
  2. Modular styles are more possible with LESS - it can import other LESS files without as much trouble as CSS, which makes for a better developer experience for maintainers of this library.
  3. Mixins are still useful and not something covered by CSS yet, as far as I'm aware.
  4. Time! I don't feel strongly enough about the above 3 to object to a change from LESS to CSS but I likely wouldn't have the capacity to do it myself, especially as it doesn't make a difference to the end-user, because we now compile LESS -> CSS in a build script.

I also ran the build script since the dist/ folder was out of date again. I might open a separate PR that does only that to merge in first, though, since then it'd be easier to see what the net effect of this PR is (including the fact that it net removes code despite adding functionality).

dwmkerr commented 8 months ago

Really nice and really nice documentation as well.

dwmkerr commented 8 months ago

@all-contributors please add @mmkal for docs, code, review

allcontributors[bot] commented 8 months ago

@dwmkerr

This project's configuration file has malformed JSON: .all-contributorsrc. Error:: Unexpected token ] in JSON at position 660

dwmkerr commented 8 months ago

@pvspain any objections to me merging this in?

dwmkerr commented 8 months ago

@mmkal would you mind updating from main? I've fixed a mistake in the 'all contributors' config which means I should then be able to use the bot to add you to the contributor list on the main page - sorry for the hassle!

mmkal commented 8 months ago

@mmkal would you mind updating from main? I've fixed a mistake in the 'all contributors' config which means I should then be able to use the bot to add you to the contributor list on the main page - sorry for the hassle!

Yes, will do later tonight (EST time(!)). I want to merge from main anyway, now that #45 is in, so the diff is easier to grok.

mmkal commented 8 months ago

@dwmkerr done!

pvspain commented 8 months ago

Thanks @mmkal. I'm happy with this @dwmkerr if you want to pull the trigger?

dwmkerr commented 8 months ago

Yep looks great thanks @mmkal and @pvspain !

dwmkerr commented 8 months ago

@all-contributors please add @mmkal for code, docs, review

allcontributors[bot] commented 8 months ago

@dwmkerr

I've put up a pull request to add @mmkal! :tada: