feat(digital-guide): add accessibility modes dialog #537
Conversation
simon-the-shark
requested review from
thesun901,
mikolaj-jalocha,
24bartixx and
tomasz-trela
simon-the-shark
force-pushed
the
feat/accessibility-mode-dialog
branch
from
a1431fa to
f38e00d
Compare
lib/features/digital_guide_view/tabs/accessibility_dialog/presentation/red_dialog.dart
Outdated
Show resolved
Hide resolved
lib/features/digital_guide_view/tabs/accessibility_dialog/presentation/red_dialog.dart
Outdated
Show resolved
Hide resolved
lib/features/digital_guide_view/tabs/accessibility_dialog/presentation/mode_checkbox.dart
Show resolved
Hide resolved
lib/features/digital_guide_view/tabs/accessibility_dialog/presentation/checkboxes_list.dart
Show resolved
Hide resolved
lib/features/digital_guide_view/tabs/accessibility_dialog/presentation/checkboxes_list.dart
Outdated
Show resolved
Hide resolved
|
This looks like significant part of code :-) I will look into that carefully but currently short on time. Will do that till tomorrow's weekly ;) |
simon-the-shark
force-pushed
the
feat/accessibility-mode-dialog
branch
from
f38e00d to
b27f380
Compare
There was a problem hiding this comment.
LGTM! Clean, understandable code that will make possible future upgrades not painful at all (everything we'd need to do is to add next class to sealed wrapper).
I'm huge fan of this although I suppose for totally fresh devs such solution is complex and not obvious to understand at first glance. In our app, that's in constant development, architecture that enable us to freely extend existing features is imo priority and there's not such a thing as an overkill.
There was a problem hiding this comment.
I like this. In Android native environment this "style" of coding is extremely common making features less painful to extend in the future.
| padding: const EdgeInsets.symmetric(horizontal: 20), | ||
| child: Text( | ||
| subtitle, | ||
| style: context.aboutUsTheme.body.copyWith( |
There was a problem hiding this comment.
Someday, someone will be really surprised that after some theme changes in aboutUs some random dialog in buildings information will also change :D
(we should organise our themes and other UIConfigs before we end digital_guide section)
There was a problem hiding this comment.
true xd but our designers are using random fonts
There was a problem hiding this comment.
also the paddings need some common design system
| controlAffinity: ListTileControlAffinity.leading, | ||
| title: Text( | ||
| mode.localizedLabel(context), | ||
| style: context.aboutUsTheme.body, |
There was a problem hiding this comment.
the same theme issues imo, but overall its acceptable for now
There was a problem hiding this comment.
yeah, it's a problem but in half of the app
There was a problem hiding this comment.
if you want, you can try to unify some shit
There was a problem hiding this comment.
but imo the paddings are even more crucial than fonts
lib/features/digital_guide_view/tabs/accessibility_dialog/presentation/red_dialog.dart
Outdated
Show resolved
Hide resolved
simon-the-shark
dismissed stale reviews from thesun901 and mikolaj-jalocha
via
dd576a5
simon-the-shark
force-pushed
the
feat/accessibility-mode-dialog
branch
from
b27f380 to
dd576a5
Compare
so when it comes to code, i went kinda crazy...
cause this could have been easily hardcoded, and I'm not sure if the layers of abstractions are not too much a bit for the use case. Also some functional programming concepts were used, allowing us to define any number of custom accessibility modes and nest them into each other in any way we want. So please tell me if this is still readable or would you prefer something more hardcoded in a way, and not fully abstract.
this could be usefull to read before the review: https://dart.dev/language/class-modifiers#sealed