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

fix: Increase dashboard RefreshIndicator edge offset #1859

Merged
merged 2 commits into from
Jun 29, 2024

Conversation

Domenic-MZS
Copy link
Contributor

@Domenic-MZS Domenic-MZS commented Apr 7, 2024

🔧🎨 UI Fix: Increase dashboard RefreshIndicator edge offset

This change adjusts the edge offset for the RefreshIndicator in the home view dashboard UI with the CustomSliverAppBar maxExtent size, ensuring a better user experience by moving the RefreshIndicator under the SliverAppbar.

Fixes #1568

Overview

The HomeView / Dashboard [CustomScrollView] refresh functionality has been enhanced by matching the starting refresh position with the [SliverAppBar] height, and by reducing the displacement range to settle the refresh.

  • Issue: The RefreshIndicator starts the animation on top of the [SliverAppBar], disturbing the user experience (?).
  • Context: The current functionality lacks an increased starting point ([edgeOffset]) or a better layout.
  • Related Solutions: (None).
  • Impact: Improves the user experience by making the refresh indicator more concise and easy to use by reducing the needed displacement to work and by correct positioning the indicator on the affected area (the body and not the header).

📓 Description

This PR sets the Dashboard [edgeOffset] (indicator starting point) to match the [CustomSliverAppBar] (title) height and changes the default [displacement] to execute the refresh callback.

✍🏼 Changes Made

  • Added property ([edgeOffset])[edge-offset].
  • Reduces the refresh indicator needed [displacement] to settle.
Technical Overview > (For the RefreshIndicator widgtet wrapping the content) - Add the `edgeOffset` property with the `CustomSliverAppBar` hardcoded `extendedHeight`. (~magic number) - Add the `displacement` property to 50. (reduces the needed scroll to settle the "refresh" action)

🦯 Testing

  • Update Tests (ToDo - Not Found nor needed for now...)
  • Compile/Build code
    • 🐞 Tested with Stable Flutter v3.19.3 and dart 3.3.1.
    • System: Linux archlinux 6.7.9-arch1-1 GNU/Linux
WhatsApp.Video.2024-04-07.at.19.39.42.mp4

📋 Notes & References

RefreshIndicator - edgeOffset
Current Custom Sliver App Bar - ExpandedHeight

🔬 Reviewers

N/A (any mod is ok)


👾 Feedback, changes, or suggestions are welcome! ✌🏼 🏖️ this time an 🥑 emoji...

This change adjusts the edge offset for the RefreshIndicator in the home
view dashboard UI with the CustomSliverAppBar `expandedHeight` size,
ensuring a better user experience by moving the RefreshIndicator under
the SliverAppbar.
@Domenic-MZS
Copy link
Contributor Author

Domenic-MZS commented Apr 7, 2024

Opened as draft as I don't know if using "magic numbers" (actually a static number related to another static magic number) is right or not...

I'm using a static edgeOffset based on the static CustomSliverAppBar expanded height, Line Number 22

return SliverAppBar(
pinned: true,
expandedHeight: 100.0,
automaticallyImplyLeading: !isMainView,

@ILoveOpenSourceApplications

Can the refresh icon animation be also improved or is it because the manager refreshes too quickly that the animation is getting canceled out?

@validcube validcube linked an issue Apr 8, 2024 that may be closed by this pull request
4 tasks
@Domenic-MZS
Copy link
Contributor Author

Domenic-MZS commented Apr 10, 2024

Can the refresh icon animation be also improved or is it because the manager refreshes too quickly that the animation is getting canceled out?

The animation is not cancelled; it's just fast because the completion of the promise for refresh happens almost instantly. This occurs because the code doesn't wait for the promises.

Future<void> forceRefresh(BuildContext context) async {
_managerAPI.clearAllData();
_toast.showBottom(t.homeView.refreshSuccess);
initialize(context);
}

@oSumAtrIX
Copy link
Member

Is this ready for review?

@Domenic-MZS

This comment was marked as outdated.

@Domenic-MZS Domenic-MZS marked this pull request as ready for review April 10, 2024 17:33
@Aunali321
Copy link
Contributor

@Domenic-MZS Can you try to remove magic numbers if possible?

@Domenic-MZS
Copy link
Contributor Author

Domenic-MZS commented Apr 17, 2024

@Domenic-MZS Can you try to remove the magic numbers if possible?

While it's technically feasible, the current trade-offs involved wouldn't add significant value. Incorporating this functionality while maintaining the current user interface would unnecessarily increase complexity for this particular case.

I can think one of three possible solutions:

  1. Make the extendedHeight of the CustomSliverAppBar parametrizable to match the offset.
  2. Encapsulate the entire component tree within a LayoutBuilder to obtain metrics and sizes after rendering, which would allow us to compute and adjust the offset during a second redraw. / Or use a NestedScrollView (Way different of the current UI as it's always scrollable)
  3. Utilize a GlobalKey to retrieve the expanded height of the SliverAppBar after it renders and then update the offset on a subsequent redraw.

However, none of these solutions are particularly appealing, as the requirement is specific to only one file, and the drawbacks outweigh the benefits.


In addition, the CustomSliverAppBar widget will most likely not change frequently, and if it does change in the future, the current implementation is most likely to work.

The most realistic scenario would be that in the future the hardcoded expandedHeight value will instead become a default which could be override. Thus, making the current implementation still functioning.

Sure, there's still a need to ensure that updates on the shared widget don't cause disruptive changes over time, but most likely, it wouldn't happen.

@oSumAtrIX oSumAtrIX removed their request for review June 4, 2024 15:45
@oSumAtrIX oSumAtrIX changed the title fix(ui): increase dashboard RefreshIndicator edge offset fix: Increase dashboard RefreshIndicator edge offset Jun 29, 2024
@oSumAtrIX oSumAtrIX merged commit 232b702 into ReVanced:dev Jun 29, 2024
1 check passed
github-actions bot pushed a commit that referenced this pull request Jun 30, 2024
# [1.21.0-dev.6](v1.21.0-dev.5...v1.21.0-dev.6) (2024-06-30)

### Bug Fixes

* Add missing import to patch options field ([d60f9aa](d60f9aa))
* Follow system theme immediately ([#1942](#1942)) ([694f2a9](694f2a9))
* Handle selecting files and folders for patch options correctly ([#1941](#1941)) ([b26760b](b26760b))
* Increase dashboard RefreshIndicator edge offset ([#1859](#1859)) ([232b702](232b702))
* Select previously applied patches when loading patch selection ([#1865](#1865)) ([7ef8f04](7ef8f04))
* Unsupported patch toast says "patchItem.unsupportedPatchVersion" ([#2011](#2011)) ([3209c0e](3209c0e))

### Features

* Save last patched app ([#1414](#1414)) ([7720408](7720408))
github-actions bot pushed a commit that referenced this pull request Jul 29, 2024
# [1.21.0](v1.20.1...v1.21.0) (2024-07-29)

### Bug Fixes

* Add missing import to patch options field ([d60f9aa](d60f9aa))
* Adjust scroll from clipping children form fields in `AlertDialog` from `showSourcesDialog` ([#1782](#1782)) ([bbeb836](bbeb836))
* Cache external API calls  ([#1911](#1911)) ([2c3e2e6](2c3e2e6))
* Change problematic translation string ([6b03f3a](6b03f3a))
* Correct architecture to armeabi-v7a ([63c6412](63c6412))
* Download latest integrations non-pre-release ([4a72267](4a72267))
* Follow language update immediately ([#1944](#1944)) ([c13827e](c13827e))
* Follow system theme immediately ([#1942](#1942)) ([694f2a9](694f2a9))
* Handle selecting files and folders for patch options correctly ([#1941](#1941)) ([b26760b](b26760b))
* Increase dashboard RefreshIndicator edge offset ([#1859](#1859)) ([232b702](232b702))
* Patching Screen draw-behind Navigation Bar ([#1945](#1945)) ([f1b25d0](f1b25d0))
* Restore consistency with the app ([ea9654e](ea9654e))
* SecurityException when patching application ([#1856](#1856)) ([e0a6de2](e0a6de2))
* Select previously applied patches when loading patch selection ([#1865](#1865)) ([7ef8f04](7ef8f04))
* Unable to install application regardless of preference ([c7627ce](c7627ce))
* Unsupported patch toast says "patchItem.unsupportedPatchVersion" ([#2011](#2011)) ([3209c0e](3209c0e))
* Update dialog shows dev version & loading gets stuck in certain circumstances ([#1792](#1792)) ([fc52560](fc52560))

### Features

* Add ability to set `null` in patch options ([#1947](#1947)) ([5c68d51](5c68d51))
* Include primary architecture in external search ([#2068](#2068)) ([23690a9](23690a9))
* open browser when clicking on changelog link ([bc300d8](bc300d8))
* Save last patched app ([#1414](#1414)) ([7720408](7720408))
* Support patching on ARMv7a ([a766352](a766352))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feat(Refresh Animation): Improve the refreshing animation
5 participants