Skip to content
This repository was archived by the owner on Jun 17, 2024. It is now read-only.

Bug 1807131: Fix ripples for snackbar and alert dialogs#1226

Merged
mergify[bot] merged 2 commits intomozilla-mobile:mainfrom
czlucius:fix-1807131
May 15, 2023
Merged

Bug 1807131: Fix ripples for snackbar and alert dialogs#1226
mergify[bot] merged 2 commits intomozilla-mobile:mainfrom
czlucius:fix-1807131

Conversation

@czlucius
Copy link
Contributor

@czlucius czlucius commented Mar 14, 2023 •
edited by github-actions bot
Loading

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1807131

@github-actions
Copy link
Contributor

github-actions bot commented Mar 14, 2023

🚧 Commit message is using the wrong format: Add ripple XML

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

@czlucius
Copy link
Contributor Author

czlucius commented Mar 17, 2023 •
edited
Loading

Screenshot_20230314_154415_Firefox Fenix
Screenshot_20230314_152821_Firefox Fenix
Screenshot_20230314_153652_Firefox Fenix

Here are some screenshots, I forgot to put them earlier.
Thanks

@czlucius
Copy link
Contributor Author

czlucius commented Apr 17, 2023 •
edited
Loading

@csadilek would like to ask you on this. What do you think of this change?
Sorry for the ping by the way, there wasn't anyone to look at this for about 1 month.
Thanks

Amejia481
Amejia481 approved these changes May 15, 2023
View reviewed changes
Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

Awesome job!
Thanks for the patch!

@Amejia481 Amejia481 force-pushed the fix-1807131 branch 2 times, most recently from 817513f to 7eead68 Compare May 15, 2023 19:34
@Amejia481 Amejia481 added 🛬 needs landing PRs that are ready to land approved PR that has been approved labels May 15, 2023
Amejia481
Amejia481 reviewed May 15, 2023
View reviewed changes
fenix/app/src/main/res/drawable/ripple_button.xml
Comment on lines +2 to +4
<!-- This Source Code Form is subject to the terms of the Mozilla Public
- License, v. 2.0. If a copy of the MPL was not distributed with this
- file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
Copy link
Contributor

@Amejia481 Amejia481 May 15, 2023

Choose a reason for hiding this comment

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

@czlucius I added the licenses to each files, also squashed the commits into a single one.

@mergify mergify bot merged commit a071055 into mozilla-mobile:main May 15, 2023
@czlucius
Copy link
Contributor Author

czlucius commented May 16, 2023

Thanks

@czlucius
Copy link
Contributor Author

czlucius commented May 16, 2023

@delia-pop I tested it on the update today (from the Play Store), and it seems to work on light mode as well. Can you confirm this behaviour? However, I'd like to file a new issue as the ripple seems to be off-centre, which did not happen when I was testing this change on my device before the patch was merged.

I can't post on Bugzilla since the issue is closed

@czlucius
Copy link
Contributor Author

czlucius commented May 16, 2023

@delia-pop The above image is for alert(), bookmarks show the ripple just fine:
image

@delia-pop
Copy link

delia-pop commented May 17, 2023 •
edited
Loading

Hi, @czlucius , thanks for reaching out!
Upon further testing, I can confirm that the ripple animation is displayed (visible) on snackbars and alerts in Light theme as well, with an exception.

1684333379914

  • on Google Pixel 6 (Android 13), Google Pixel 7 (Android 14) and Google Pixel 7 PRO (Android 14), the animation is barely visible with Light theme, on both snackbars and alerts. However, this is not the case for Google Pixel 7 (Android 13) where the animation is visible.
DialogueButtonAnimation.mp4

Since this seems a device issue, please let me know if you think we should file a ticket for this behavior anyway.

Regarding the issue you mentioned, with the ripple animation not centered on the "OK" button on alerts, I can also reproduce it and I will shortly open a ticket for it. Here's the ticket: https://bugzilla.mozilla.org/show_bug.cgi?id=1833661

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@Amejia481 Amejia481 Amejia481 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved PR that has been approved 🛬 needs landing PRs that are ready to land

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@czlucius @delia-pop @Amejia481