[Feat] Load position in board editor #1394
Conversation
|
Hi @veloce I need some advice on how to land this feature. First of all speaking about the UX. imo navigating to a full screen selection list is far way better than using modals when many items are present, let me know if you disagree. Other than that, I would like to have some indication on how to organize the code for a proper data fetching implementation. Thanks |
|
Yes, full screen selection is fine! Will look at the code asap. |
There was a problem hiding this comment.
I think your implementation if perfectly fine. Using StatefulWidgets instead of providers in that case is valid. You could have used providers, but I don't think we'd gain anything in using providers compared to StatefulWidgets. Also, I don't expect these positions to be loaded elsewhere in the app in the future.
Good job!
| child: Text(context.l10n.loadPosition), | ||
| onPressed: () { | ||
| final notifier = ref.read(editorController.notifier); | ||
| pushReplacementPlatformRoute( |
There was a problem hiding this comment.
This function doesn't exist anymore. You should update to the last main commit. Now we're using directly Navigator.of(context).push and we define route as static method of Screen type widgets using the buildScreenRoute function.
| } | ||
|
|
||
| class _OpeningsTabState extends State<_OpeningsTab> { | ||
| // TODO Provider |
| _openings = DefaultAssetBundle.of(context).loadString('assets/positions.json').then((s) { | ||
| final List<Position> result = []; | ||
| for (final opening in jsonDecode(s) as List) { | ||
| // ignore: avoid_dynamic_calls |
There was a problem hiding this comment.
better to remove that and use as List<Map<String, dynamic>> instead.
| } | ||
|
|
||
| class _EndGamesTabState extends State<_EndGamesTab> { | ||
| // TODO Provider |
| void initState() { | ||
| _endGames = DefaultAssetBundle.of(context).loadString('assets/endgames.json').then((s) { | ||
| final List<Position> result = []; | ||
| for (final position in jsonDecode(s) as List) { |
There was a problem hiding this comment.
cast the explicit type of List.
|
Thanks @veloce, my point about the providers was just to easily cache data loaded and parsed from files. |
3c5b410 to
591c50f
Compare
Yes sure we could do that, but I don't believe caching this data is important. |
| this.leading, | ||
| this.actions = const [], | ||
| this.androidTitleSpacing, | ||
| this.bottom, |
There was a problem hiding this comment.
Thanks for letting me know about this feature of CupertinoNavigationBar I wasn't aware of ;)
Now putting that in PlatformScaffold might lead to some issue. The way the platform scaffold wraps PlatformBottomBar in PreferredSize is not ideal, I will refactor this. Also I tested the bottom property of CupertinoNavigationBar, and it still has some issues with routes transition, currently being fixed in flutter repo.
I think PlatformScaffold should stay very limited, and don't support too much use cases. For now I'd say lets remove the bottom from platform scaffold and use separately Scaffold and CupertinoPageScaffold in your PR.
4c06d2c to
2bb177a
Compare
|
Hi @veloce. I made some changes based on your comments. |
Hi there, this PR contains a raw implementation of the load position feature.
closes #991