search

kognise / water.css

A drop-in collection of CSS styles to make simple websites just a little nicer
https://watercss.kognise.dev
MIT License
8.33k stars 494 forks source link

Next Steps #187

Closed kognise closed 4 years ago

kognise commented 4 years ago

EDIT: Here's the current checklist:

As far as I can see, here's the current state of Water.css 2.0:

I'd love feedback on what we should do about these things but here's what I've been thinking:

I think we should stop including the dist folder within the repository and instead rely on a continuous integration platform, potentially github actions, to build the code and deploy it to both a git tag and npm. This will make contributing easier and ensure that existing sites get updates.

I'm not entirely sure about this, but we might want to consider completely remaking the documentation site and build pipeline with something slightly less janky. We could also consider deploying the site to a different service, maybe vercel? This will give the added benefit of being able to gracefully switch over to the new documentation.

Furthermore, a lot of the issues are simply caused by multiple pull requests that have overlap and merge conflicts - I propose instead we create one monster pr that when merged will update everything.

Sorry for the huge block of text, please tell me if I was unclear in any way!

cc @kylejrp @kimulaco @jonaskuske @JackFly26

kimulaco commented 4 years ago

Thanks for the draft of the next step!

I think we should stop including the dist folder within the repository and instead rely on a continuous integration platform, potentially github actions, to build the code and deploy it to both a git tag and npm. This will make contributing easier and ensure that existing sites get updates.

I have additional opinions related to this.

I think it is better to create a release note when managing versions with npm in the future.γ€€Because, the user has more strict control over the version of water.css. Use semantic-release or similar library, you can automatically perform the following update work. (I don't have a deep understanding of semantic-release, so we need to consider what to use.)

kognise commented 4 years ago

That sounds a lot better! Meanwhile, I'm having trouble getting @jonaskuske's pull request with the new gulp workflow working.

When I run gulp it says gulp isn't installed. When I install gulp, it says gulp-postcss isn't installed even though both of them are in the package.json. But when I install gulp-postcss, gulp gets uninstalled. Any idea what's conflicting? Maybe I'm just being stupid.

$ file node_modules/gulp
node_modules/gulp: directory

$ yarn add gulp-postcss --dev
(Redacted)
Done in 1.85s.

$ file node_modules/gulp
node_modules/gulp: cannot open `node_modules/gulp' (No such file or directory)
kylejrp commented 4 years ago

I'm on my phone so this is a little brief, I can dig in deeper later.

  • There are several conflicting pull requests relating to the documentation site, and it's likely that merging the wrong ones will completely break the site.

I can go through these and merge or close where appropriate.

  • The css file that the site references is out of sync with the one that's currently in the readme, and the readme references some features that aren't yet in the site.

If I remember correctly, there's two sites - one on Netlify and one still on GitHub Pages. The GitHub Pages site is out of date for sure and should probably be a redirect to the Netlify site.

  • We're currently included the built css files in the repository and managing versions with git tags, but this makes contributing more complex and pull requests significantly harder to merge.

I think this is my fault, we removed the dist folder at one point and then I accidentally merged in an old change that still had the dist files. There should be a pull request for this from me.

  • The plan is to use npm for version management in the future, but then existing sites won't receive updates unless we also continue pushing git tags.

There's a discussion on one of the issues about how jsdelivr will always be using v1.0, which is okay because we're making breaking changes in v2.0. I think that this is because we renamed water.css to light.css and dark.css. We would want to provide a CDN distribution that uses the appropriate new versions as the source of truth, and links to v2.x.x instead of the absolute latest in case we have to make breaking changes again.

  • The development workflow, currently based on gulp, isn't ideal and has a few issues.

Gulp isn't there for any specific reason other than its what I suggested when I first joined the project. I agree that it's not ideal. What would you recommend?

  • We're currently using netlify to deploy the documentation site but there have been some concerns raised about their new policies.

Are you able to look up the usage information for the site? I wonder if we're even affected by the changes.

I'd love feedback on what we should do about these things but here's what I've been thinking:

I think we should stop including the dist folder within the repository and instead rely on a continuous integration platform, potentially github actions, to build the code and deploy it to both a git tag and npm. This will make contributing easier and ensure that existing sites get updates.

GitHub actions is a great idea. Automating git releases is also good.

I'm not entirely sure about this, but we might want to consider completely remaking the documentation site and build pipeline with something slightly less janky. We could also consider deploying the site to a different service, maybe vercel? This will give the added benefit of being able to gracefully switch over to the new documentation.

Furthermore, a lot of the issues are simply caused by multiple pull requests that have overlap and merge conflicts - I propose instead we create one monster pr that when merged will update everything.

I disagree, I like having manageable pull requests that are easy to review and merge as we complete tasks. I would be looking to close stale pull requests and reduce the scope on existing pull requests that have been too hard to complete. The smaller scope that each pull request has, the quicker we can get it merged!

We will also want to make sure we're happy with all of the breaking changes we're making before releasing a 2.0.

I'll see if I can edit this comment later with links to the real issues/pull requests I mentioned...

jonaskuske commented 4 years ago

Hmm, I start to understand why some fb folks constantly complain about GitHub's lack of staggered PRs on Twitter πŸ˜„

Tough choice – I'd really like to see v2 get done and published and just creating one huge PR, somehow merging everything in more or less one swoop would certainly help there (actually I was considering to do just that in my own fork in the last couple days so interesting timing on this!)... But individually re-visiting, fixing and merging the individual PRs would allow us to re-assess them and check what parts we may want to update or omit for v2 which I think is necessary, so agree with @kylejrp here. Case in point: I'm pretty sure my version picker PR still includes some custom <details>/<summary> styles because water.css didn't have official support yet when I first built it. And we could also re-visit and potentially streamline the version/naming strategy there.

@kognise What exactly are you doing before those gulp/gulp-postcss problems arise? Did you merge something already? If I just check out my feat/version-select branch and run yarn, everything compiles without issues.

Oh, and as for publishing: some good recommendations in this recent thread, mostly semantic-release and atlassian/changesets.

As for where the docs are hosted, I don't care, as long as we can still get per-PR preview URLs. (sucks that GitHub Pages can't do this...) Are we actually close to hitting the 100GB/300 build minutes per month on Netlify?

kylejrp commented 4 years ago

Here are the action items I can find for you @kognise:

After merging the above, most of the 2.0 requirements should be fixed. Then, all that's left is:

kognise commented 4 years ago

Thanks for taking a look at this!

There's a discussion on one of the issues about how jsdelivr will always be using v1.0, which is okay because we're making breaking changes in v2.0. I think that this is because we renamed water.css to light.css and dark.css. We would want to provide a CDN distribution that uses the appropriate new versions as the source of truth, and links to v2.x.x instead of the absolute latest in case we have to make breaking changes again.

I disagree - I think we should continue pushing git tags so that existing sites running @latest will still get 2.0. If we have to we can copy some of the build files around to maintain backwards compatibility with the filenames.

I also agree with @jonaskuske, but imo the pros outweigh the cons of making one huge pull request.

I want to avoid making changes to master for as long as possible, so what about this:

Then, what @kylejrp said:

Then just:

Thoughts?

kylejrp commented 4 years ago

I disagree - I think we should continue pushing git tags so that existing sites running @latest will still get 2.0. If we have to we can copy some of the build files around to maintain backwards compatibility with the filenames.

Note that the original docs asked users to use water.css@latest and they haven't been getting updates as we switched to dark.css@latest and light.css@latest. I guess our docs have been showing the new links for a while though - so you're right that we should be maintaining compatibility for those using the newer jsdelivr links.

the pros outweigh the cons of making one huge pull request

It's different means to the same end so it all works for me!

I want to avoid making changes to master for as long as possible

If we're avoiding breaking master anymore, I think it would be important to merge in your proposed 2.0 branch sooner than later. After that, we should have a clear branching strategy to prevent master from becoming a mess again.

I think that just merging in #148, then #140 and #90 will get us most of the way there with the least amount of effort.

kognise commented 4 years ago

If we're avoiding breaking master anymore, I think it would be important to merge in your proposed 2.0 branch sooner than later. After that, we should have a clear branching strategy to prevent master from becoming a mess again.

100% agreed!

I just pushed the two branch and I'll get started merging the prs you mentioned.

jonaskuske commented 4 years ago

I also think we should keep publishing git tags, if only because devs expect to find the version history (and changelogs!) on the releases page on GitHub. But yeah, that means that the moment we add the v2 git tag, users will be hit by breaking changes. (but only then, I think we never recommeneded installing via @master)

For that reason I'd also suggest we recommend installation via @1, @2 or whatever the current major is from now on, instead of @latest πŸ˜…

kylejrp commented 4 years ago

Awesome, thanks @kognise! I'm really excited to see what we can do to make water.css the best it can be. 😁

For that reason I'd also suggest we recommend installation via @1, @2 or whatever the current major is from now on, instead of @latest πŸ˜…

This! I'd hate to make breaking changes in the future that break a slew of websites that are on plain old @latest. πŸ˜…

kognise commented 4 years ago

Ight! Am I correct that this is all we have to do?

[checklist was here]

I'm sorry for dragging this on for so long, thanks for sticking with it.

I've also removed #89 from the 2.0 milestone because I agree with @kylejrp that it isn't really related to the launch.

EDIT: Moved the checklist to the top of the issue.

kylejrp commented 4 years ago

You're on fire! πŸ”₯πŸ”₯πŸ”₯ Awesome work!

Now's the time to review if we're happy with how the files are structured - it's harder to take back something we've shipped. We have dark.css, dark_standalone.css and dark.legacy.css. We should evaluate if we still want these names or if we want to drop some. I think that @jonaskuske said that the standalone doesn't make sense. I think we need the legacy one still for IE11 compatibility.

kognise commented 4 years ago

Naming is really hard! What about dark-fallback.css, dark-forced.css, and dark-legacy.css? But then we lose dark.css which means any semblance of backwards compatibility goes bye bye.

kylejrp commented 4 years ago

I like keeping dark.css as an opinionated default. It's fine to keep that as using the native system theme.

kognise commented 4 years ago

Makes sense, what do you think of dark-forced and dark-legacy? I also think it might be a good idea to update the ui to clarify that dark is just a fallback.

kylejrp commented 4 years ago

I don't have any strong preference towards a specific name, I'm just more worried about if we should be having a file. @jonaskuske mentioned that the standalone might not make sense now that there's stronger browser support for prefers-color-scheme. I think I'd be looking for their advice on what versions we should keep around now.

jonaskuske commented 4 years ago

Heh yep, thanks for the work on this @kognise @kylejrp!!

I'm just more worried about if we should be having a file. @jonaskuske mentioned that the standalone might not make sense now that there's stronger browser support for prefers-color-scheme.

Hmm, I guess this was a misunderstanding – I just think that letting devs choose between Dark and Light for the default version would not make sense any more, because for 90% of users this choice would be overridden by (prefers-color-scheme) anyway. So I suggested that we update the UI to make it clear that the choice devs have only affects the fallback scheme that's displayed for users on old browsers... But now that I think about it, this is such an unimportant thingΒΉ that in my opinion we could just not offer a choice here at all and instead decide ourselves whether light or dark is the fallback in old browsers. However I do think that having "standalone" versions is a good idea, some devs might not want the styling to change between light and dark unexpectedly, especially if they're extending it somehow with their own CSS.

So this would leave us with these options in the UI:

Theme: [x] Auto [] Dark [] Light Support IE? [x]

And my suggestion for the naming scheme would be (auto|dark|light)[-ie].css.


ΒΉ Since the dev is using the default/auto version, they don't 100% care which theme the user sees anyway, so why control the fallback? And if someone desperately needs a specific theme in old browsers, but the auto one in modern browsers, they're more than welcome to solve that in userland – we got the Theming section for a reason :)

kognise commented 4 years ago

I've been reading through the documentation site's code and although Vue is very nice it seems like it's a bit overboard, especially since we're simplifying the version selector so much.

I'm going to make a separate branch for updating the code and file structure and then make a PR for a review opportunity.

jonaskuske commented 4 years ago

Yup, I initially reached for it because a declarative approach made coordinating the 8 different versions, their respective tooltip messages, code snippets and form state along with the transitions and the feedback for the copy button easier. (adding types with JSdoc was 100% unnecessary though πŸ˜…)

But yeah we can probably simplify all this and get rid of it. (but we should definitely keep the Copy button and I also still like the transitions :) )

kognise commented 4 years ago

@jonaskuske should we keep the naming as legacy instead of ie? The normal versions should support ie11. Also, am I correct that in the legacy versions auto mode won't be supported?

jonaskuske commented 4 years ago

The normal versions don't support IE11 as they use CSS variables to apply their theme. And yep, auto mode in IE doesn't work as IE does not know about the (prefers-color-scheme) media query, auto-ie.css will always show whatever we decide is the default/fallback theme in IE (and other browsers that don't know about (prefers-color-scheme)).

image

kylejrp commented 4 years ago

Proposal: auto.css should just be the original water.css

IE11 also doesn't support CSS variables. If we wanted to keep IE11 compatibility in the main distribution, I think we could do some overloading like:

div {
    color: #abc123;
    color: var(--primary);
}

IE11 will use the fallback to the explicit hex declaration and ignore the second because it doesn't know the var function. Modern browsers will use the second declaration that uses the css variable.

I think this would eliminate the legacy distribution and leave us with just water.css, dark.css, and light.css. πŸŽ‰

kognise commented 4 years ago

I'm still not sure if auto.css would be more clear than water.css but I'll look into the best way to do the overloading.

My other concern is that the legacy autoprefixer will add a looot of bloat to what would otherwise be a small bundle.

kylejrp commented 4 years ago

I'm just trying to stay on brand with the project name πŸ˜‰

It would be interesting to look what we're gaining or losing from autoprefixing

kognise commented 4 years ago

Here are the size changes for dark.css:

css variables: dark.css gained 4.37 kB (7.97 kB -> 12.34 kB)
autoprefixer: dark.css gained 379 B (12.34 kB -> 12.71 kB)
minification: dark.css saved 5.05 kB (12.74 kB -> 7.69 kB)

Looks like we're only losing 0.85kb with the autoprefixer and variable fallback inlining.

kylejrp commented 4 years ago

The variable fallback inlining should also gzip really nicely since it's all duplicated content. I think jsdelivr does gzipping.

I think that less than 1kb is totally acceptable for the kind of change that reduces the number of files we distribute.

kognise commented 4 years ago

I was going to make the PR and finish most of the other things in the checklist but unfortunately a power outage hit

On Wed, May 27, 2020, 9:01 PM Kyle Pollard notifications@github.com wrote:

The variable fallback inlining should also gzip really nicely since it's all duplicated content. I think jsdelivr does gzipping.

I think that less than 1kb is totally acceptable for the kind of change that reduces the number of files we distribute.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kognise/water.css/issues/187#issuecomment-635047658, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKEVYGJN7K2K2TVCMRU3XELRTXAWVANCNFSM4NLJNVSA .

kylejrp commented 4 years ago

Ouch! ⚑ No worries at all. Thanks again for all your hard work!

kognise commented 4 years ago

Linking this to #191.