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

Don't allow participant list insertion if the chairman is not present #388

Merged
merged 5 commits into from
Dec 21, 2022

Conversation

lagartoverde
Copy link
Contributor

No description provided.

@lagartoverde lagartoverde self-assigned this Dec 14, 2022
@lagartoverde lagartoverde added the enhancement New feature or request label Dec 14, 2022
Copy link
Member

@abeforgit abeforgit left a comment

Choose a reason for hiding this comment

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

Just a smal QoL comment, works well otherwise
OK to merge for me, moving the alert is optional

@@ -5,6 +5,14 @@
as |Modal|>
<Modal.Body>
<div class="au-o-flow">
{{#if this.error}}
Copy link
Member

Choose a reason for hiding this comment

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

I would put this error next to the "insert" button

atm when you are scrolled down, you don't see the error and it feels like the insert button just doesnt work

Copy link
Member

@abeforgit abeforgit left a comment

Choose a reason for hiding this comment

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

come to think of it, wouldn't it be better to simply disable the present toggle if the mandatee = the chairman?

We have no backup flow for when they do set the chairman as absent, we simply don't allow them to proceed. So why even have the option in the first place?
edit: looking at the ticket, that seems to be what they're asking anyway

@lagartoverde
Copy link
Contributor Author

I have made the changes you suggested, also kept the message and moved the error just in case, or if we want to add more errors to it

Copy link
Contributor

@elpoelma elpoelma left a comment

Choose a reason for hiding this comment

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

Works as expected.

@lagartoverde lagartoverde merged commit 8792ce5 into master Dec 21, 2022
@lagartoverde lagartoverde deleted the feature/chairman-validator branch December 21, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants