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

Add support for all CSS features #159

Merged
merged 6 commits into from
Jun 8, 2023

Conversation

RJWadley
Copy link
Contributor

@RJWadley RJWadley commented May 14, 2023

This pull request adds a script that gets updates the database and creates test and implementation stubs for any new CSS features. I've used these stubs to add support for every CSS feature in the caniuse-lite database.

Since this package seems abandoned, I've published these changes on npm as @rjwadley/doiuse. I'll be maintaining the package there until either I or someone else can maintain it here.

This PR also depends on #157, since the changes in that pull request made this script possible. That PR's changes are included in this one. That also means the diff isn't very useful right now. For a better view of the changes, here's a more focused diff with only the relevant changes.

New Features

Changes

Fixes

#8
#76
#90
#99
#100
#102
#107
#139
#140
#142
#143
#144
#145
#155

Supersedes

#148
#156

@RJWadley
Copy link
Contributor Author

RJWadley commented May 27, 2023

When #157 was merged it was merged by rebase, which meant all those commits appeared twice in this PR. As such, I've had to also rebase and force-push this branch and my other branches to remove the duplicates.

Whomever merges this, please just use a standard merge commit to prevent also breaking my other work. Thanks!

@clshortfuse
Copy link
Collaborator

Hi! Can you rebase?

Also it's best if the changes are a bit more atomic for better review.

I'm okay with multiple commits but the descriptions should be semantic/conventional. In other words separated by intent (fix, feat, docs, test, etc). For example, see https://gist.github.com/joshbuchea/6f47e86d2510bce28f8e7f42ae84c716 I'm not picky about intent names, but some at-a-glance commit descriptions would be helpful.

I hope this doesn't deter you from contributing and I really appreciate the effort you've put in. I know organizing and squashing git commits can be tedious.

Thanks again!

@RJWadley
Copy link
Contributor Author

No worries. I'm not too experienced with rebase, but I'll see what I put together for ya.

If you'd like to enforce that sort of style, you might should set up commit linting and update CONTRIBUTING.md, since the repo doesn't really have a history of using semantic commits before you.

@RJWadley
Copy link
Contributor Author

RJWadley commented May 30, 2023

@clshortfuse I've done a rebase and cleaned things up best I can. I can also bring the version back to 6.0.0 if you want, since doiuse hasn't released that yet.

@clshortfuse
Copy link
Collaborator

Thanks! It's much easier to read now. I'm also pushing changes, so apologies for breaking the rebase 😅

The commit changes shouldn't affect package.json, or package-lock.json unless there's something specifically in those new versions that is required to run. It'll make more sense when we do a one shot update of all dependencies or individual ones (more than one of this PR's commit is updating caniuse).

You should also not touch CHANGELOG.md because your commit details should have what I would need to compile into the published version. Basically changes for git users are in the commits. Changes for npm users are in CHANGELOG.md. It's only when I make the packaged version that I make the modification to CHANGELOG.md. That allows me to cherry-pick certain things and even support multiple versions (eg: backport something to the 5.x branch).

So, when I review, I'm just looking at what folders were touched and if tests pass. I don't want to worry about runtime issues if you're only touching /test and /data. I look more closely at changes done to /lib and /utils.

In the end, this will land as one commit feat: add more css support. That's the at-a-glance description. In the details you can explain (if you want) that we're going to use a script to generate the feature list and compile tests off that.

I think with the changelog/package.json changes we should be good to merge. I'll push another commit that does those changes (including the contributor change =) )

@RJWadley
Copy link
Contributor Author

RJWadley commented May 30, 2023

I went ahead and dropped the changelog & package stuff. That commit also removed the travis badge, since CI runs on github actions now. I'll leave removing that up to you as well.

I made the feature script update caniuse, so that package got updated a lot while I was iterating. Sorry if that made things harder to read.

Would you prefer PRs only contain a single commit? I can totally do that if that's what you prefer. Or were you planning to squash this yourself when you merged?

@clshortfuse
Copy link
Collaborator

I was going to squash it into a single commit since each commit is based on work of the previous and stays within the same intention of adding support for all CSS features.

I'm okay with multiple commits generally and squash them before commit, though one with this many file changes can get tricky to review. But that's for future reference.

It seems there are still package.json changes. If squashing it into one commit helps you find where it's still making those changes, that's fine as well.

@RJWadley
Copy link
Contributor Author

RJWadley commented Jun 5, 2023

The remaining package.json changes are:

  • The caniuse-lite update necessary to detect these new features
  • Addition of feature update under "scripts"

Would you like both of these reverted?

@clshortfuse
Copy link
Collaborator

Sorry, I wasn't notice they were essential. For future reference, a commit that just updates package.json by itself would probably be better. The language of the commit doesn't matter as much.

I'll go ahead and merge it. I have some internal changes and I want to make this part of 6.0.0. Thanks again for your work!

clshortfuse added a commit that referenced this pull request Jul 7, 2023
* Use regex for all CSS values

Regression from #159 causes all rule values containing `vi` to be validated against `viewport-unit-variants`.

Fixes #164
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.

2 participants