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

Sync button touches the card's bottom edge #274

Merged

Conversation

Prakhar-Agarwal-byte
Copy link
Contributor

Fixes #272
Screenshot:
WhatsApp Image 2023-03-23 at 10 57 24 AM

@thunderbiscuit thunderbiscuit added the ui User interface label Apr 2, 2023
@thunderbiscuit thunderbiscuit added this to the v1.0.0 milestone Apr 2, 2023
Copy link
Owner

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

It's weird how little is written about the LocalMinimumTouchTargetEnforcement API.

I haven't worked a lot with CompositionLocalProvider, but from what I can see its a way to affect every node in the tree below a certain point/node. If I understand correctly, the sync button is the only node that requires this LocalMinimumTouchTargetEnforcement to be set to false, and in that sense I think it would be best to find the lowest point that we can add this composition local provider. From my quick testing this appears to be at the button level directly (line 217). Unless you have a reason to have it at a higher level, let's bring down the

CompositionLocalProvider(
    LocalMinimumTouchTargetEnforcement provides false,
)

so that it wraps only the button. Othewise this looks good! You're rebased onto master so if you make that change this is ready to go in.

@thunderbiscuit thunderbiscuit self-requested a review April 3, 2023 18:57
Copy link
Owner

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK 3e2ac4b.

@thunderbiscuit thunderbiscuit merged commit 3e2ac4b into thunderbiscuit:master Apr 3, 2023
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui User interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sync button should touch the edge of the balance card
2 participants