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

Adding column rules #389

Merged
merged 3 commits into from
Aug 26, 2021
Merged

Adding column rules #389

merged 3 commits into from
Aug 26, 2021

Conversation

fidian
Copy link
Contributor

@fidian fidian commented May 19, 2021

Adds column CSS rules.

Closes #371

@redonkulus redonkulus requested a review from src-code May 21, 2021 17:19
@redonkulus
Copy link
Contributor

@fidian thanks for the PR, looks like the unit tests need to be updated too

@fidian
Copy link
Contributor Author

fidian commented May 21, 2021

I have no idea how to fix the test. Probably something is wrong with the newly added rules, but I don't seem to be able to find it. I don't see how .Bd or .BdT would match Colmr.

@src-code
Copy link
Contributor

@fidian The issue seems to be that you're missing a matcher property for Colmr

src/rules.js Outdated
*/
{
'type': 'pattern',
'name': 'Colm',
Copy link
Contributor

Choose a reason for hiding this comment

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

name property should be an English-readable string that succinctly describes the rule, see other rules for examples

src/rules.js Outdated
},
{
'type': 'pattern',
'id': 'Colmr',
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need id

Copy link
Contributor

@src-code src-code left a comment

Choose a reason for hiding this comment

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

Marking this blocked until it's updated

@redonkulus
Copy link
Contributor

@fidian you still interested in getting this in?

@fidian
Copy link
Contributor Author

fidian commented Aug 22, 2021

Yeah, I am still interested in getting this in.

Without any mean intents, I have wondered the same of the atomizer maintainers. The changes were simple once @src-code pointed them out and anyone could have made it happen. Is this project mostly abandoned? I find it extremely valuable.

@redonkulus
Copy link
Contributor

Typically we review the code expecting the PR creator to update the code based on the review suggestions.

We are still maintaining atomizer and use it at Yahoo today. The code base is stable and generally we will add new features as requirements arise.

If you still want to get this in, please update the code based on @src-code suggestions. Thanks!

@fidian
Copy link
Contributor Author

fidian commented Aug 23, 2021 via email

@redonkulus
Copy link
Contributor

Ok I see the commit but the tests still need updating. You can follow how the other ones are done.

@fidian
Copy link
Contributor Author

fidian commented Aug 26, 2021

I pushed a merge of the current master branch, but the tests all run for me. I don't know what to say because the tests all appear to work, even after running npm ci and npm run test.

image

@redonkulus redonkulus requested a review from src-code August 26, 2021 15:53
@src-code src-code merged commit 9a1762c into acss-io:master Aug 26, 2021
@redonkulus
Copy link
Contributor

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.

Add support for columns
3 participants