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

Next Steps #187

Closed
6 of 7 tasks
kognise opened this issue May 26, 2020 · 29 comments · Fixed by #194
Closed
6 of 7 tasks

Next Steps #187

kognise opened this issue May 26, 2020 · 29 comments · Fixed by #194
Assignees
Labels
question Further information is requested
Milestone

Comments

@kognise
Copy link
Owner

kognise commented May 26, 2020

EDIT: Here's the current checklist:

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

  • 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.
  • 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.
  • 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.
  • 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.
  • The development workflow, currently based on gulp, isn't ideal and has a few issues.
  • We're currently using netlify to deploy the documentation site but there have been some concerns raised about their new policies.

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

@kognise kognise added the question Further information is requested label May 26, 2020
@kognise kognise added this to the 2.0 Launch milestone May 26, 2020
@kognise kognise changed the title Next steps Next Steps May 26, 2020
@kognise kognise pinned this issue May 26, 2020
@kimulaco
Copy link
Contributor

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

  • Publish to npm.
  • Create a git tag.
  • Create a release note.

@kognise
Copy link
Owner Author

kognise commented May 27, 2020

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
Copy link
Collaborator

kylejrp commented May 27, 2020

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
Copy link
Collaborator

jonaskuske commented May 27, 2020

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
Copy link
Collaborator

kylejrp commented May 27, 2020

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:

  • See if there's any other breaking changes we want to make
  • Add any extra functionality required for a 2.0 launch
  • Review the docs / demo to make sure everything is looking great
  • Publish to npm with semantic versioning

@kognise
Copy link
Owner Author

kognise commented May 27, 2020

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:

  • I'm gonna create a branch called two which is a copy of @jonaskuske's pr with master merged in
  • I'll update the Netlify config
  • We can work on merging other pull requests into that newly created branch
  • We'll figure out ci and versioning

Then, what @kylejrp said:

  • See if there's any other breaking changes we want to make
  • Add any extra functionality required for a 2.0 launch
  • Review the docs / demo to make sure everything is looking great

Then just:

  • Merge everything into master in one giant pr
  • Publish to npm and github with semantic versioning

Thoughts?

@kylejrp
Copy link
Collaborator

kylejrp commented May 27, 2020

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
Copy link
Owner Author

kognise commented May 27, 2020

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
Copy link
Collaborator

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
Copy link
Collaborator

kylejrp commented May 27, 2020

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

This was referenced May 27, 2020
@kognise
Copy link
Owner Author

kognise commented May 27, 2020

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
Copy link
Collaborator

kylejrp commented May 27, 2020

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
Copy link
Owner Author

kognise commented May 27, 2020

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
Copy link
Collaborator

kylejrp commented May 27, 2020

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

@kognise
Copy link
Owner Author

kognise commented May 27, 2020

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
Copy link
Collaborator

kylejrp commented May 27, 2020

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
Copy link
Collaborator

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
Copy link
Owner Author

kognise commented May 27, 2020

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
Copy link
Collaborator

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
Copy link
Owner Author

kognise commented May 27, 2020

@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
Copy link
Collaborator

jonaskuske commented May 28, 2020

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
Copy link
Collaborator

kylejrp commented May 28, 2020

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
Copy link
Owner Author

kognise commented May 28, 2020

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
Copy link
Collaborator

kylejrp commented May 28, 2020

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
Copy link
Owner Author

kognise commented May 28, 2020

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
Copy link
Collaborator

kylejrp commented May 28, 2020

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
Copy link
Owner Author

kognise commented May 28, 2020 via email

@kylejrp
Copy link
Collaborator

kylejrp commented May 28, 2020

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

@kognise
Copy link
Owner Author

kognise commented May 28, 2020

Linking this to #191.

@kognise kognise mentioned this issue May 30, 2020
@kognise kognise unpinned this issue Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants