Skip to content
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

refactor: factor out user-selection widget for actions #2426

Merged
merged 2 commits into from Nov 5, 2020

Conversation

opqdonut
Copy link
Contributor

@opqdonut opqdonut commented Nov 4, 2020

related to #2040

Definition of Done / Review checklist

Reviewability

  • link to issue

Also drop the now-unused :t.actions/request-selection translation. The
request-review dialog should've been using the plural form all along
anyway.
:item-key :userid
:item-label :display
:item-selected? #(contains? (set selected-deciders) %)
:multi? true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here, decider selection has had multi? true. The event & command also support multiple users.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it could potentially be wrong. We don't have it as configurable and I guess that's ok for now, but the assumption we had in an earlier discussion couple weeks ago (when doing the wording change) was that there would only ever be one decider.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which wording change? Looking at the history, the label has been :t.actions/request-selection and multi? has been true at least since 2019-08-01, commit a6c15aa.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! What do you propose? I find that the UI is clear enough even with multiselection, and it doesn't really do any harm to support multiple deciders.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I would just like to confirm from @jaakkocsc that it's so far ok to have multiple deciders. The changes look fine otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say the decision on having only one decider or multiple deciders is organisational, by the "organisation-owner", not a technical limitation set by REMS.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It simplifies REMS to have all the invites/requests to be available for multiple people.

Copy link
Collaborator

@Macroz Macroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just check that the change wrt. decider requests / wording is ok but otherwise let's merge.

@opqdonut opqdonut merged commit 56dcfd1 into master Nov 5, 2020
@opqdonut opqdonut deleted the refactor-actions-2 branch November 5, 2020 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants