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

refactor(tables): use pseudo private custom properties #1204

Merged
merged 10 commits into from
Jan 5, 2023

Conversation

dancormier
Copy link
Contributor

@dancormier dancormier commented Dec 5, 2022

This PR refactors .s-table component styling to use pseudo-private custom properties. With this refactor, I didn't flatten the cascade for variants/modifiers entirely as I have with many other refactors. Tables include some very targeted styles (targetingfirst/last/nth of nested elements) that would've made a complete* flattening more confusing and hard to maintain than the alternative.

Note
This PR should result in no visual changes for the .s-table component.


* "complete" as consistent, not as in total or done.

@dancormier dancormier added the work-in-progress A work in progress, not meant to merge label Dec 5, 2022
@dancormier dancormier requested a review from a team December 5, 2022 22:39
@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit b8167bf
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/6398a0dc082c48000935d241
😎 Deploy Preview https://deploy-preview-1204--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ondrejkonec
Copy link

Hi @dancormier

I see that you are refactoring table component and there is Issue that I tried to fix.

So maybe there is chance now for merging my PR, if you feel it make sense.

@dancormier dancormier marked this pull request as ready for review December 7, 2022 22:53
@dancormier dancormier removed the work-in-progress A work in progress, not meant to merge label Dec 8, 2022
Copy link
Contributor

@giamir giamir 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 @dancormier!
There are a couple of small fixes to address which I have highlighted in my comments.
Apart from that this looks good to me. I don't think I need to re-review after you have applied the changes - feel free to go ahead and merge. 🙂

lib/css/components/tables.less Outdated Show resolved Hide resolved
lib/css/components/tables.less Outdated Show resolved Hide resolved
docs/assets/less/stacks-documentation.less Outdated Show resolved Hide resolved
@dancormier
Copy link
Contributor Author

Thanks for another great review @giamir! It really helps to have your eyes on these. Much like #1206, I'm holding on merging for now until Core is updated to 1.6.x.

@dancormier
Copy link
Contributor Author

Core's been updated. Time to merge!

@dancormier dancormier merged commit 70a93a9 into develop Jan 5, 2023
@dancormier dancormier deleted the dcormier/table-refactor branch January 5, 2023 16:48
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

3 participants