-
Notifications
You must be signed in to change notification settings - Fork 155
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
Make the whole items in advanced settings screen clickable, standarize paddings #2314
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2314 +/- ##
========================================
Coverage 70.08% 70.09%
========================================
Files 1353 1351 -2
Lines 33247 33230 -17
Branches 6877 6871 -6
========================================
- Hits 23301 23291 -10
+ Misses 6638 6633 -5
+ Partials 3308 3306 -2 ☔ View full report in Codecov by Sentry. |
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.
Interesting. I am trying to see what can cause this.
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.
OK, so I think removing this line will fix this regression. Glad to have screenshot test!
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 will match what we have in CheckableUserRow
(which is missing a Preview...)
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 should be fixed in 7c794f5.
Let's see the result in the screenshots.
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.
Now I am wondering if the fix could be to add the padding where it was missing, instead of removing the existing one. The Checkbox are closer to the edge now... Maybe add a 8.dp
padding?
Also maybe CheckableUnresolvedUserRow
and CheckableUserRow
could use a common Composable generic layout.
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.
True, I didn't realise that. The 8dp
padding will probably solve it, yes 👍 .
Also maybe CheckableUnresolvedUserRow and CheckableUserRow could use a common Composable generic layout.
I can try to make a common one.
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.
A small regression to fix, else LGTM. Thanks for working on this!
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.
Thanks for the update. I do not see any more issue in the screenshot.
Feel free to ignore my last comment.
Also increase end padding to try to match previous layout.
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Type of change
Content
ListItem
components always have the minimum interactive size (48x48dp) to fix screenshot tests discrepancies seen with the changes above.Sadly, this created more discrepancies, but at least now it's standarized.
Motivation and context
UX issues discovered while testing this screen.
Screenshots / GIFs
There are quite a few in the PR contents.
Tested devices
Checklist