Skip to content

Conversation

@Akshaykomar890
Copy link
Contributor

@Akshaykomar890 Akshaykomar890 commented Jan 8, 2025

Fixes #5463

Disabled Hint: Set hintEnabled="false" in the WCMaterialOutlinedSpinnerView style to prevent the floating hint from occupying space and causing unintended ripple effects in the layout.

Before:
WhatsApp Image 2025-01-08 at 11 59 24 PM

After:
WhatsApp Image 2025-01-08 at 11 59 22 PM

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

Disabled Hint: Set hintEnabled="false" in the WCMaterialOutlinedSpinnerView style to prevent the floating hint from occupying space and causing unintended ripple effects in the layout.
@Akshaykomar890 Akshaykomar890 changed the title Fix Spinner Ripple Effect in WCMaterialOutlinedSpinnerView Fix Spinner Ripple Effect in WCMaterialOutlinedSpinnerView. #5463 Jan 8, 2025
@Akshaykomar890 Akshaykomar890 changed the title Fix Spinner Ripple Effect in WCMaterialOutlinedSpinnerView. #5463 Fix Spinner Ripple Effect in WCMaterialOutlinedSpinnerView. Jan 8, 2025
@malinajirka malinajirka self-assigned this Jan 9, 2025
@malinajirka malinajirka added the category: design Layout and style elements in the UI or user interface, including color and animations. label Jan 9, 2025
@malinajirka malinajirka added this to the 21.4 milestone Jan 9, 2025
Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Thank you so much @Akshaykomar890!

It wouldn't have crossed my mind this was caused by the hint, good catch! Seeing it now it makes total sense.

This change unfortunately breaks all the spinners that are setting a hint. For example:

Before After
Screenshot_1736420594 Screenshot_1736420551

Steps to reproduce:

  1. Tap on Products
  2. Tap on Plus icon
  3. Tap on Simple Subscription product
  4. Tap on Add Price

Understanding the reason now, I can't think of a solution. Update: Perhaps we could create a custom background-ripple drawable with top padding, wdyt? I'm not sure whether it'd work as expected for different font-size(accessibility settings).

What about you, do you have some ideas?

@Akshaykomar890
Copy link
Contributor Author

Hi @malinajirka ,

Thank you for pointing this out and for the detailed steps to reproduce the issue.

You're absolutely right that we can try creating a custom ripple with top padding. However, I believe it might not be fully stable across different font sizes and accessibility settings. While it's worth exploring,

I have another approach in mind:
Alternatively, we can leverage Material 3 by using:
https://github.com/material-components/material-components-android/blob/master/docs/components/Menu.md
<com.google.android.material.textfield.TextInputLayout
android:id="@+id/menu"
style="@style/Widget.Material3.TextInputLayout.FilledBox.ExposedDropdownMenu" >

this approach address issue without introducing instability. Let me know what you think, and I’m happy to refine the solution further.

@malinajirka
Copy link
Contributor

Thanks @Akshaykomar890, replacing all the components with different components introduces a risk of regression which is IMO not worth it for such a small UI glitch most users won't even notice. I'd suggest either testing the approach with padding (it doesn't need to necessarily be perfect, as long as it's an improvement) or marking this issues as won't fix. What are your thoughts on all this?

@Akshaykomar890
Copy link
Contributor Author

Thanks for your detailed feedback @malinajirka I completely understand the concern about regression risks. I'll test the padding adjustment approach and see if it provides a noticeable improvement. If the results aren't significant, I'm fine with marking this as "Won't Fix" for now. Thanks again.

@Akshaykomar890
Copy link
Contributor Author

Akshaykomar890 commented Jan 10, 2025

hello @malinajirka
I’ve tried the custom padding approach and added a commit with the changes. Could you please check if it meets the criteria? I believe this should improve the ripple effect without introducing any major issues.

You can find the commit here:e0599ff

WhatsApp Image 2025-01-10 at 3 16 59 PM WhatsApp Image 2025-01-10 at 3 16 58 PM

I’ve tested it both with and without hints, and it’s working as expected in both cases.

Looking forward to hearing your thoughts!

@wpmobilebot wpmobilebot modified the milestones: 21.4, 21.5 Jan 10, 2025
@wpmobilebot
Copy link
Collaborator

Version 21.4 has now entered code-freeze, so the milestone of this PR has been updated to 21.5.

Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this @Akshaykomar890!

I’ve tested it both with and without hints, and it’s working as expected in both cases.

Nice, thanks for sharing what you tested!

I also tested light/dark mode and different font size and it all looks good to me. It's tiny bit off with custom font sizes as we anticipated, but it's still an improvement compared to the previous state.

Thank you for contributing and congrats on your first merged PR in this repo! 🚢

@malinajirka malinajirka enabled auto-merge January 10, 2025 11:53
@Akshaykomar890
Copy link
Contributor Author

Thank you so much @malinajirka for testing it thoroughly. I'm glad the changes have improved the previous state, and it’s exciting to contribute to the repo. I thankful for your guidance and support throughout the process.

Should I wait for this PR to merge, or can I start working on a new issue in the meantime?
Looking forward to contributing more.

@malinajirka
Copy link
Contributor

Should I wait for this PR to merge, or can I start working on a new issue in the meantime? Looking forward to contributing more.

Nice, thank you! Feel free to pick up any amount of issues in parallel you like.

@malinajirka malinajirka merged commit 7fa5ee8 into woocommerce:trunk Jan 13, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: design Layout and style elements in the UI or user interface, including color and animations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adjust ripple effect area in WCMaterialOutlinedSpinnerView

3 participants