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

More css vars - take 2 #44

Merged
merged 17 commits into from
Oct 23, 2023
Merged

More css vars - take 2 #44

merged 17 commits into from
Oct 23, 2023

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Oct 19, 2023

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).

@mmkal mmkal mentioned this pull request Oct 19, 2023
@dwmkerr
Copy link
Owner

dwmkerr commented Oct 19, 2023

Really nice and really nice documentation as well.

@dwmkerr
Copy link
Owner

dwmkerr commented Oct 19, 2023

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

@allcontributors
Copy link
Contributor

@dwmkerr

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

@dwmkerr
Copy link
Owner

dwmkerr commented Oct 19, 2023

@pvspain any objections to me merging this in?

@dwmkerr
Copy link
Owner

dwmkerr commented Oct 19, 2023

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

mmkal commented Oct 19, 2023

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

mmkal commented Oct 19, 2023

@dwmkerr done!

Copy link
Contributor

@pvspain pvspain left a comment

Choose a reason for hiding this comment

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

Hi @mmkal. This looks great - thanks!

There's four things I'd like you to have a look at, regarding a couple of clobbered values and documentation. (We need a section on documentation!) They are all commented below.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
dev/index.less Show resolved Hide resolved
style/crosswords.less Show resolved Hide resolved
Copy link
Contributor

@pvspain pvspain left a comment

Choose a reason for hiding this comment

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

This look good @mmkal.

A comment in there to leave a reference to styling in the README. That should have been there already - my bad.

README.md Outdated Show resolved Hide resolved
docs/crossword-styling.md Show resolved Hide resolved
@mmkal mmkal requested a review from pvspain October 21, 2023 03:00
@pvspain
Copy link
Contributor

pvspain commented Oct 21, 2023

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

@dwmkerr
Copy link
Owner

dwmkerr commented Oct 23, 2023

Yep looks great thanks @mmkal and @pvspain !

@dwmkerr
Copy link
Owner

dwmkerr commented Oct 23, 2023

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

@allcontributors
Copy link
Contributor

@dwmkerr

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

@dwmkerr dwmkerr merged commit 1c656e1 into dwmkerr:main Oct 23, 2023
2 checks passed
@mmkal mmkal deleted the more-css-vars branch October 23, 2023 19:23
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.

3 participants