-
Notifications
You must be signed in to change notification settings - Fork 9
feat(cat-voices): Add collaborators section to proposal builder #3803
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
base: feat/co-proposers-3677
Are you sure you want to change the base?
feat(cat-voices): Add collaborators section to proposal builder #3803
Conversation
📚 Docs PreviewThe docs for this PR can be previewed at the following URL: https://docs.dev.projectcatalyst.io/voices/feat/proposal-builder-collaborators |
...packages/internal/catalyst_voices_models/lib/src/document/specialized/proposal_document.dart
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
catalyst_voices/apps/voices/lib/pages/proposal_builder/proposal_builder_segments.dart
Outdated
Show resolved
Hide resolved
.../apps/voices/lib/pages/proposal_builder/widgets/proposal_builder_document_collaborators.dart
Show resolved
Hide resolved
…rc/document/specialized/proposal_document.dart Co-authored-by: Ryszard Schossler <[email protected]>
...nal/catalyst_voices_repositories/lib/src/document/source/database_documents_data_source.dart
Show resolved
Hide resolved
.../apps/voices/lib/pages/proposal_builder/widgets/proposal_builder_document_collaborators.dart
Outdated
Show resolved
Hide resolved
.../apps/voices/lib/pages/proposal_builder/widgets/proposal_builder_document_collaborators.dart
Outdated
Show resolved
Hide resolved
.../apps/voices/lib/pages/proposal_builder/widgets/proposal_builder_document_collaborators.dart
Outdated
Show resolved
Hide resolved
...es/apps/voices/lib/pages/proposal_builder/widgets/proposal_builder_collaborators_action.dart
Outdated
Show resolved
Hide resolved
| void _onCollaboratorsChanged(BuildContext context) { | ||
| final tileController = DocumentBuilderSectionTileControllerScope.of(context); | ||
| final dataNodeId = _CollaboratorsDetails.getDataNodeId(property.nodeId); | ||
| final collaborators = tileController.getData<List<CatalystId>>(dataNodeId) ?? []; | ||
| final event = UpdateCollaboratorsEvent(collaborators: collaborators); | ||
| context.read<ProposalBuilderBloc>().add(event); | ||
| tileController.removeData(dataNodeId); | ||
| } |
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.
In general we don't want to look inside other class to extract data that somebody else wrote, you should only read what you yourself wrote. Instead we should get the data we are interested in directly and explicitly in the callback. Here you're overriding the onChanged which only works with document changes so you cannot pass the data you're really interested in.
I do not have yet any ready solution thought through but I'd see if it's possible to avoid above and just communicate with callbacks
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.
The source of this problem is how _CollaboratorsDetailsSelector is built and that it is wrapped in DocumentBuilderSectionTile->EditableTile->DocumentPropertyBuilder, which gives us certain benefits but also the onChanged methods are not adapted to custom behaviors.
I tried to disable the save button for EditableTile and create my own inside _CollaboratorsDetailsSelector, but then there was a problem with refreshing the state of widgets outside (DocumentBuilderSectionTile and EditableTile).
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.
What if next to onChanged we had onCollaboratorsChanged and to propagate this callback through all these layers? I'm afraid that with current solution it's too easy to change what we write in one place but forget to update the code that reads these values in somewhere else.
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.
Done, I think it looks much better now. Pulling out and editing collaborators data is more centralized.
catalyst_voices/apps/voices/lib/widgets/user/accounts_text.dart
Outdated
Show resolved
Hide resolved
...oices/packages/internal/catalyst_voices_view_models/lib/src/catalyst_id/catalyst_id_ext.dart
Outdated
Show resolved
Hide resolved
damian-molinski
left a comment
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.
LGTM
Description
This Pull Request implements the Collaborators feature in the Proposal Builder, allowing users to invite and manage co-proposers.
Related Issue(s)
Closes #3683
Part of #3677
Description of Changes
collaboratorsto the schema:Demo
Adding the same
CatalystIdwith a changedusernameis not allowed, and there is a task for that.collaborators_builder.mov
Please confirm the following checks