-
Notifications
You must be signed in to change notification settings - Fork 4
fix: prevent scrolling bug in digital guide #976
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
Conversation
simon-the-shark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this looks acceptable, but I would try one more thing to see if we could get same results without custom imperative cache, if you'd allow
| AccessibilityInformationCard createCard({ | ||
| required String icon, | ||
| required int level, | ||
| required String? comment, | ||
| required String peopleLabel, | ||
| }) { | ||
| return AccessibilityInformationCard( | ||
| icon: icon, | ||
| color: DigitalGuideConfig.accessibilityLevelColors[level], | ||
| text: | ||
| comment ?? | ||
| context.localize.accessibility_card_information( | ||
| prefix, | ||
| accessibilityLevelType(level.toString()), | ||
| peopleLabel, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a function when it can be a private widget (this is builder methods anti-pattern)?
you can use inheritance if you don't want to repeat the build methods
class AccessibilityLevelCard extends AccessibilityInformationCard {
_AccessibilityLevelCard({
required BuildContext context,
required String icon,
required int level,
required String? comment,
required String peopleLabel,
required String prefix,
super.key,
}) : super(
icon: icon,
color: DigitalGuideConfig.accessibilityLevelColors[level],
text: comment ??
context.localize.accessibility_card_information(
prefix,
accessibilityLevelType(level.toString()),
peopleLabel,
),
);
}(or a new widget with build method is 100% ok as well)
| Map<ModeWithKey, AccessibilityInformationCard> _getCardsMap( | ||
| BuildContext context, { | ||
| required AccessibilityInformationLevelsInput levels, | ||
| required AccessibilityInformationOptionalCommentsInput? comments, | ||
| required String prefix, | ||
| required String Function(String) accessibilityLevelType, | ||
| }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of creating a method that returns a map I would just perfer to have a declarative-style switch (even inline in the build)
| return currentModesAsync.when( | ||
| loading: () => buildCards(cache), | ||
| error: (error, stack) => Center(child: Text("Error: $error")), | ||
| data: buildCards, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I'm not saying that the cache you've built is not needed. It might be. We can go with it if it is needed truly. But could you try if something like that WITHOUT custom imperative cache that at least I believe should have very similar effects to what you've had done with the cache here:
switch (currentModesAsync) {
AsyncError(:final error, :final stackTrace) => // error widget,
AsyncValue(:final value) when value != null => buildCards(value) // this case should cover both, normal `AsyncData` case and `AsyncLoading` which has value from a previous render (that's how riverpod at least worked in the 2.0 version, not sure how about 3.0),
_ => // actual loading cause no data is present (value == null),
};afaik this should use the previous value even when refreshing so as far as i understand your code, very similar thing. not sure if 1:1 but very similar. could you try?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- if the
keepAlive: trueis crucial you should probably add it toactiveAccessibilityModesRepositoryor whatever is needed
805b714 to
77dd3d6
Compare
|
I managed to do this changing only riverpod repo |
The issue was that the accesibility widget with cards was had circular progress indicator when it was loading data. Then the indicator was changing to SizedBox.shink()
I implemented cache to store previous accesibility that is fetched asynchrounously causing this loading.
Moreover, I refactor the accesibiltiy list to load the card widgets lazily and used column instead of ListView (it's already inside CustomScrollView and there is not that many cards to render them dynamically imo) for perf.
Important
Implements caching and refactors accessibility list to prevent scrolling bug and improve performance in
active_accessibility_modes_repository.dart.ActiveAccessibilityModesRepositoryto store previous accessibility data, preventing unnecessary re-fetching.Columninstead ofListViewfor performance improvement.activeAccessibilityModesRepositoryto a classActiveAccessibilityModesRepositorywith caching logic inactive_accessibility_modes_repository.dart.This description was created by
for 77dd3d6. You can customize this summary. It will automatically update as commits are pushed.