-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
feat: Improve photo gallery accessibility + internationalization #5366
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5366 +/- ##
==========================================
- Coverage 9.54% 7.43% -2.12%
==========================================
Files 325 380 +55
Lines 16411 19483 +3072
==========================================
- Hits 1567 1448 -119
- Misses 14844 18035 +3191 ☔ View full report in Codecov by Sentry. |
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.
Hi @g123k!
It's no obvious in your UI if the date refers to the top or the bottom image.
And I have doubts about letting translators decide technical date formats and I recommend DateFormat.yMd
.
Beyond those remarks, it's an improvement, thanks!
Ping @jusdekiwi who created #5128 (#5144).
Cards would make sense 👍 |
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.
Hi @g123k!
That's almost perfect, but I would really simplify it and dismiss your palette related code, cf. my comments.
final DateFormat dateFormat = | ||
DateFormat.yMd(ProductQuery.getLanguage().offTag); | ||
|
||
darkBackground = darkBackground ?? true; |
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 default is true
, it would be much simpler to say that the default is true, wouldn't it?
Something like bool darkBackground = true;
. Et voilà.
Same for backgroundColor
.
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.
Here we can say it's a balance between optimization VS code clarity.
By doing bool darkBackground = true
, we force all items in the grid to have a value in the memory.
Here, until the palette is computed, it's memory free.
_loadImagePalette(); | ||
} | ||
|
||
Future<void> _loadImagePalette() async { |
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.
I don't believe in your palette tap dancing, for several UI/UX reasons:
- dark mode means dark mode
- let's assume that the user wants to have widgets with the same color when they do the same thing on the same page, and not a "random" meaningless patchwork of dark and light
- you're somehow downloading the full image twice - even though I guess there's cache involved behind
- the colors may change after loading is complete
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.
It's better than the current situation, approved.
HI everyone,
Here is the current layout for the gallery:
There are three issues:
I've fixed all of them + added the icon from the POC: