Replace tasks list providers with id-based providing #2046
Merged
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
…s based on client.subscribeStream()
|
The following could fix issue that outer list page of tasklists is not updated when tasklist title changes:
Then |
… id and uses ref.watch in its rendering
…s simple id and uses ref.watch in its rendering
…s based on client.subscribeStream()
kumarpalsinh25
approved these changes
Aug 13, 2024
There was a problem hiding this comment.
Code wise look good to me. At some of place not sure about ref.invalidate removal so would request @gnunicorn to review once before it gets merged.
gnunicorn
approved these changes
Aug 13, 2024
Comment on lines
63
to
65
| return const ListTile( | ||
| title: Text('failed'), | ||
| ); |
There was a problem hiding this comment.
we should show the error for easier debugging
Suggested change
| return const ListTile( | |
| title: Text('failed'), | |
| ); | |
| return ListTile( | |
| title: Text('Loading failed: $error'), | |
| ); |
| @@ -193,13 +212,11 @@ class TaskItem extends ConsumerWidget { | |||
| ); | |||
| } | |||
|
|
|||
| Key doneKey() { | |||
| final taskId = task.eventIdStr(); | |||
| Key doneKey(String taskId) { | |||
There was a problem hiding this comment.
these are not necessary. taskId is an attribute of the widget, we don't have to pass it in.
Suggested change
| Key doneKey(String taskId) { | |
| Key doneKey() { |
| return Key('task-entry-$taskId-status-btn-done'); | ||
| } | ||
|
|
||
| Key notDoneKey() { | ||
| final taskId = task.eventIdStr(); | ||
| Key notDoneKey(String taskId) { |
There was a problem hiding this comment.
Suggested change
| Key notDoneKey(String taskId) { | |
| Key notDoneKey() { |
Comment on lines
50
to
53
| _log.severe('failed to load tasklist', error, stack); | ||
| return const Card( | ||
| child: Text('failed'), | ||
| ); |
Comment on lines
61
to
62
| Widget title(BuildContext context, TaskList taskList) { | ||
| final taskListId = taskList.eventIdStr(); |
There was a problem hiding this comment.
unnecessary, we already know taskListId.
Suggested change
| Widget title(BuildContext context, TaskList taskList) { | |
| final taskListId = taskList.eventIdStr(); | |
| Widget title(BuildContext context, TaskList taskList) { |
gnunicorn
changed the title
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.
ref.invalidate()about parts that were added after 1st version ofsubscribeStreamreplacing