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

docs: added "Thinking in Groups" section to grouping docs #69

Merged
merged 4 commits into from
Jan 9, 2021
Merged

docs: added "Thinking in Groups" section to grouping docs #69

merged 4 commits into from
Jan 9, 2021

Conversation

just214
Copy link
Collaborator

@just214 just214 commented Jan 7, 2021

This PR includes a "Thinking in Groups" section of the grouping documentation, which uses a button example to demonstrate different approaches to grouping. It also includes verbiage to address the concern mentioned in #59.

@just214 just214 mentioned this pull request Jan 7, 2021
@just214
Copy link
Collaborator Author

just214 commented Jan 8, 2021

@sastan - I am unable to add a reviewer to this PR and also not sure how to get it to pass the Github Action. If you approve of the updates to the docs, please just let me know if there is anything else that I need to do to get this properly merged. Thanks.

@sastan
Copy link
Collaborator

sastan commented Jan 8, 2021

I have seen this and i'm quite whoooa... Thank you this is really helpful.

The github action fails because of the format. Could you try to run yarn format.

I should add a pre-commit hook do this...

As i am short in time could you move this section to be one of the first or maybe the first as it is a great introduction why grouping is a powerful feature.

Copy link
Collaborator

@sastan sastan left a comment

Choose a reason for hiding this comment

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

Great work. I think this will really help people understand why grouping may help.

docs/grouping.md Outdated

Before: `bg-purple-800 hover:bg-purple-700 focus:bg-purple-700`

After: `bg(purple(800 700(hover:& focus:&)))`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think bg-purple(800 700(hover:& focus:&)) is a little bit more readable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree and it saves a character. Every bit counts!

docs/grouping.md Outdated

Before: `focus-visible:ring-4 ring-purple-400`

After: `ring(purple-400 focus-visible:(4))`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do the additional braces help here: focus-visible:(4)?

Maybe change to ring(purple-400 focus-visible:4)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fully agree. No need for the parens.

docs/grouping.md Outdated Show resolved Hide resolved
@just214
Copy link
Collaborator Author

just214 commented Jan 8, 2021

I've made the requested changes and glad to hear that this is helpful! I moved the "Thinking in Groups" section to the top because it didn't make sense to me to nest it between the sections describing grouping methods.

The GitHub action is still failing even with pre-commit formatting. This appears to be the culprit:

npm ERR! 403 403 Forbidden - PUT https://npm.pkg.github.com/@tw-in-js%2ftwind - Resource not accessible by integration

Also, I should have branched this off. I'll do that moving forward.

If you'd like to see any other changes, just let me know!

@sastan
Copy link
Collaborator

sastan commented Jan 8, 2021

Thank you. Looks good. Going to merge later.

The build error has something to do with the publishing of the preview package. I'll look into that.

@just214 just214 requested a review from sastan January 8, 2021 16:08
@sastan sastan merged commit 3a8c5e1 into tw-in-js:main Jan 9, 2021
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.

None yet

2 participants