feat(settings): add possibility for the user to change app's language (only in the settings) #648
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BombardierBulge
requested review from
mikolaj-jalocha,
simon-the-shark and
tomasz-trela
and removed request for
mikolaj-jalocha
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 8af59de in 2 minutes and 45 seconds
More details
- Looked at
230lines of code in7files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. lib/features/settings/settings.dart:23
- Draft comment:
The onTap callback for language selection has an empty if block; consider applying the language update. - Reason this comment was not posted:
Based on historical feedback, this comment is too similar to comments previously marked by users as bad.
2. lib/features/navigation_tab_view/widgets/settings_title.dart:14
- Draft comment:
FutureBuilder is used to call PackageInfo but its snapshot is never used. Consider removing FutureBuilder or handling snapshot states appropriately. - Reason this comment was not posted:
Based on historical feedback, this comment is too similar to comments previously marked by users as bad.
3. lib/features/settings/widgets/language_settings_view.dart:22
- Draft comment:
The 'selectedLanguage' variable is reinitialized on every build inside StatefulBuilder, which prevents state persistence. Move it outside the builder or use a proper stateful widget. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While technically true that the variable is reinitialized on every build, this appears intentional. The initial value always comes from the current locale, and any changes immediately trigger a Navigator.pop() which closes the dialog. There's no need for state persistence between builds since the dialog closes on selection. The StatefulBuilder is only used to trigger UI updates while the dialog is open.
The reviewer might be right that this could be done more cleanly with a StatefulWidget. There could be edge cases where rebuilds cause unexpected behavior.
However, the current implementation is functionally correct for its purpose - showing a dialog and closing it on selection. The reinitialization doesn't cause any actual issues.
The comment should be deleted as it suggests fixing something that isn't actually broken and works as intended in this specific use case.
4. lib/features/navigation_tab_view/widgets/settings_title.dart:15
- Draft comment:
Using Future.microtask(PackageInfo.fromPlatform) seems unnecessary if the package info is not used. Consider calling PackageInfo.fromPlatform() directly or removing it. - Reason this comment was not posted:
Marked as duplicate.
5. lib/features/navigator/app_router.dart:18
- Draft comment:
There appears to be a potential typographical error in the import on line 18: "digital_guide_entrace.dart". It may be intended to be "digital_guide_entrance.dart" to match the common spelling. Please verify if this is a typo or an intentional naming. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. lib/features/navigator/utils/navigation_commands.dart:157
- Draft comment:
Typographical consistency issue: In thenavigateDigitalGuideDoorfunction, the parameter is nameddoorIDbut is passed toDoorRouteasdoorsID. Consider renaming it (e.g.,doorId) to ensure consistent naming and improve code clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. lib/features/navigator/utils/navigation_commands.dart:114
- Draft comment:
Typographical note: The type and function nameDigitalGuideEntraceinnavigateEntrancesDetailsappear to contain a typo ('Entrace' instead of 'Entrance'). If this isn’t intentional, consider correcting it to improve readability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. lib/l10n/app_pl.arb:97
- Draft comment:
Typographical error: The value 'Nowa funckcjonalność' should be corrected to 'Nowa funkcjonalność'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. lib/l10n/app_pl.arb:87
- Draft comment:
Typographical error: The key 'scientific_cirlces' appears to be misspelled. It should likely be 'scientific_circles'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. lib/l10n/app_pl.arb:342
- Draft comment:
Typographical error: The key 'adapted_toilet_seat_hight' seems to be misspelled. It should be 'adapted_toilet_seat_height'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. lib/l10n/app_pl.arb:743
- Draft comment:
Typographical error: The key 'handle_heigth_text' is misspelled. It should be 'handle_height_text'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_gLVBJrKy4gBM7kp4
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| return FutureBuilder( | ||
| future: Future.microtask(PackageInfo.fromPlatform), | ||
| builder: | ||
| (context, snapshot) => NavigationTile( |
There was a problem hiding this comment.
FutureBuilder is not using the snapshot; if PackageInfo isn't needed for UI, remove FutureBuilder.
| contentPadding: const EdgeInsets.all(AboutUsConfig.spacerHeight), | ||
| content: StatefulBuilder( | ||
| builder: (context, setState) { | ||
| String selectedLanguage = context.localize.localeName; |
There was a problem hiding this comment.
Initializing selectedLanguage inside StatefulBuilder causes reinitialization on rebuild; move it outside the builder.
lib/features/settings/settings.dart
Outdated
| NavigationTile( | ||
| onTap: () async { | ||
| final selectedLang = await LanguageDialog.show(context); | ||
| if (selectedLang != null) {} |
There was a problem hiding this comment.
The onTap callback processes the language selection result but does nothing with it. Consider handling the selected language or adding a TODO comment.
Suggested change
| if (selectedLang != null) {} | |
| if (selectedLang != null) { /* TODO: Handle selected language */ } |
|
maybe add polish and english emojis to the ui? |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on a7d3b67 in 1 minute and 7 seconds
More details
- Looked at
30lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. lib/features/settings/settings.dart:24
- Draft comment:
TODO remains: Implement language change handling logic after selection. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, indicating a TODO item that remains to be implemented. It doesn't provide a suggestion or ask for confirmation about a specific code change.
2. lib/features/settings/widgets/language_settings_view.dart:55
- Draft comment:
Consider deferring or removing setState if the dialog closes immediately on tap. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. lib/features/settings/widgets/language_settings_view.dart:24
- Draft comment:
Added flag emojis are a nice UX touch. Ensure consistent emoji support across target devices. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_x95qyT66e2MA5YDM
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| onTap: () async { | ||
| final selectedLang = await LanguageDialog.show(context); | ||
| if (selectedLang != null) { | ||
| /* TODO: Handle selected language */ |
There was a problem hiding this comment.
TODO left unimplemented. Link to issue or implement handling before merging.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added settings panel and language panel
Important
Adds a settings panel with language selection capability, including new navigation routes and UI components.
SettingsViewinsettings.dartwith a language selection option.LanguageDialoginlanguage_settings_view.dartfor language selection.SettingsRoutetoapp_router.dart.navigateSettings()innavigation_commands.dart.SettingsTitlewidget insettings_title.dartto navigate to settings.navigation_tab_view.dartto includeSettingsTitle.app_pl.arbfor Polish localization.This description was created by
for a7d3b67. It will automatically update as commits are pushed.