-
Notifications
You must be signed in to change notification settings - Fork 494
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
Comments
Thanks for the draft of the next step!
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.
|
That sounds a lot better! Meanwhile, I'm having trouble getting @jonaskuske's pull request with the new gulp workflow working. When I run
|
I'm on my phone so this is a little brief, I can dig in deeper later.
I can go through these and merge or close where appropriate.
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.
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.
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.
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?
Are you able to look up the usage information for the site? I wonder if we're even affected by the changes.
GitHub actions is a great idea. Automating git releases is also good.
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... |
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. @kognise What exactly are you doing before those 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? |
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:
|
Thanks for taking a look at this!
I disagree - I think we should continue pushing git tags so that existing sites running 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
Then, what @kylejrp said:
Then just:
Thoughts? |
Note that the original docs asked users to use
It's different means to the same end so it all works for me!
If we're avoiding breaking master anymore, I think it would be important to merge in your proposed 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. |
100% agreed! I just pushed the |
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. For that reason I'd also suggest we recommend installation via |
Awesome, thanks @kognise! I'm really excited to see what we can do to make water.css the best it can be. 😁
This! I'd hate to make breaking changes in the future that break a slew of websites that are on plain old |
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. |
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 |
Naming is really hard! What about |
I like keeping dark.css as an opinionated default. It's fine to keep that as using the native system theme. |
Makes sense, what do you think of |
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 |
Heh yep, thanks for the work on this @kognise @kylejrp!!
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 So this would leave us with these options in the UI: Theme: And my suggestion for the naming scheme would be ¹ 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 :) |
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. |
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. |
@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? |
The normal versions don't support IE11 as they use CSS variables to apply their theme. |
Proposal: 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:
IE11 will use the fallback to the explicit hex declaration and ignore the second because it doesn't know the I think this would eliminate the legacy distribution and leave us with just |
I'm still not sure if My other concern is that the legacy autoprefixer will add a looot of bloat to what would otherwise be a small bundle. |
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 |
Here are the size changes for dark.css:
Looks like we're only losing 0.85kb with the autoprefixer and variable fallback inlining. |
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. |
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 ***@***.***> 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
<#187 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKEVYGJN7K2K2TVCMRU3XELRTXAWVANCNFSM4NLJNVSA>
.
|
Ouch! ⚡ No worries at all. Thanks again for all your hard work! |
Linking this to #191. |
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
The text was updated successfully, but these errors were encountered: