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

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

Merged
merged 2 commits into from May 15, 2023

Conversation

czlucius
Copy link
Contributor

@czlucius czlucius commented Mar 14, 2023

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

🚧 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

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

@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

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

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
17 checks passed
@czlucius
Copy link
Contributor Author

Thanks

@czlucius
Copy link
Contributor Author

@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

@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

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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved PR that has been approved 🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants