-
-
Notifications
You must be signed in to change notification settings - Fork 29
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 enabled to SettingsGroup & Fix padding in Sample #267
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great! Let me test it as I made this review from my phone.
As you fixed some paddings, could you provide a before / after comparative so I understand what was wrong?
...ndroidMain/kotlin/com/alorma/compose/settings/sample/shared/previews/SettingsGroupPreview.kt
Show resolved
Hide resolved
Actually, I don't add any padding. I just apply the padding Do you remember this? the |
I notice that the job Should I push the screenshots that generated by executing |
No, don't worry. New screenshots will fail, it's ok. As screenshots are generated on GHA those will fail if generated on your local. I assume that, I know those will fail when added on PRs. Let me review this PR and merge with admin power later :) |
Looks great! Thanks for your contribution! |
BTW, can you updadte README.md to reflect the enabled, change on group? |
no problem |
I added
enabled
to the SettingsGroup.Now Settings will get the default
enabled
value from the Group which directly wraps it.I fixed the padding in Sample as I was writing samples.