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 enabled to SettingsGroup & Fix padding in Sample #267

Merged
merged 7 commits into from
May 24, 2024
Merged

Conversation

TinyHai
Copy link
Contributor

@TinyHai TinyHai commented May 23, 2024

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.

@alorma alorma changed the title Add enabled to SettingsGroup & Fix paddng in Sample Add enabled to SettingsGroup & Fix padding in Sample May 23, 2024
Copy link
Owner

@alorma alorma left a 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?

@TinyHai
Copy link
Contributor Author

TinyHai commented May 24, 2024

As you fixed some paddings, could you provide a before / after comparative so I understand what was wrong?

Actually, I don't add any padding. I just apply the padding Scaffold provided to its content, so that its content will not be covered by SampleTopBar. Review App.kt

Do you remember this? the SettingsSwitch was covered by SampleTopBar. My work is to rescue the poor SettingsSwitch

@TinyHai
Copy link
Contributor Author

TinyHai commented May 24, 2024

I notice that the job Validate screenshots is failling.

Should I push the screenshots that generated by executing ./gradlew updateScreenshotTest?

@alorma
Copy link
Owner

alorma commented May 24, 2024

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 :)

@alorma
Copy link
Owner

alorma commented May 24, 2024

Looks great! Thanks for your contribution!

@alorma alorma merged commit 913fa83 into alorma:main May 24, 2024
2 checks passed
@alorma
Copy link
Owner

alorma commented May 24, 2024

BTW, can you updadte README.md to reflect the enabled, change on group?

@TinyHai
Copy link
Contributor Author

TinyHai commented May 24, 2024

BTW, can you updadte README.md to reflect the enabled, change on group?

no problem

@TinyHai TinyHai deleted the dev branch May 24, 2024 14:49
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