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

Participants editable by editors #1639

Merged

Conversation

knollengewaechs
Copy link
Collaborator

fixes #1569 (hopefully)

@knollengewaechs knollengewaechs force-pushed the participants_editable_by_editors branch 3 times, most recently from fb05859 to 9356846 Compare October 18, 2021 18:16
Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

Thanks for contributing :)

evap/contributor/forms.py Outdated Show resolved Hide resolved
evap/static/scss/_adjustments.scss Outdated Show resolved Hide resolved
evap/static/scss/_adjustments.scss Outdated Show resolved Hide resolved
@knollengewaechs knollengewaechs force-pushed the participants_editable_by_editors branch from 42b280c to 2410ce2 Compare November 22, 2021 21:42
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

the user select field for me looks like this:
image

i assume it looks better for you - have you tested it on the current main branch and did you update the bootstrap submodule and the npm sass installation?

evap/contributor/forms.py Outdated Show resolved Hide resolved
@knollengewaechs knollengewaechs force-pushed the participants_editable_by_editors branch from 35ea686 to 645fa50 Compare December 6, 2021 21:01
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

✔️ Meets requirements
✔️ UI functionality checked

evap/static/scss/_adjustments.scss Show resolved Hide resolved
@janno42 janno42 requested a review from niklasmohrin December 13, 2021 16:56
Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

I haven't really checked whether everything that needs to be deleted actually is deleted, but the diff looks good at least :)

evap/static/scss/_adjustments.scss Show resolved Hide resolved
evap/contributor/forms.py Outdated Show resolved Hide resolved
evap/contributor/tests/test_forms.py Show resolved Hide resolved
evap/contributor/tests/test_views.py Outdated Show resolved Hide resolved
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

I just remembered one thing that needs to be done in this PR as well (sorry).
Similar to the button requesting a new account in the contributor section, we need the same button (opening the same modal) also for participants, in case editors want to add someone who doesn't have an account yet.

@knollengewaechs
Copy link
Collaborator Author

knollengewaechs commented Jan 24, 2022

I just remembered one thing that needs to be done in this PR as well (sorry). Similar to the button requesting a new account in the contributor section, we need the same button (opening the same modal) also for participants, in case editors want to add someone who doesn't have an account yet.

Is the button only needed when editors are allowed to edit?
Should the button be under the request changes button or next to the participant field?

@janno42
Copy link
Member

janno42 commented Jan 31, 2022

The button should only be shown when editors are allowed to edit. It should optimally be placed next to the participant field but I assume this isn't easy. So it is also okay to place it where the request changes button would be if editors aren't allowed to edit.

@janno42
Copy link
Member

janno42 commented Jan 31, 2022

yes, please :)
the following form field should have at least the same distance to the button as the button has to the top of the card.

@knollengewaechs
Copy link
Collaborator Author

yes, please :) the following form field should have at least the same distance to the button as the button has to the top of the card.

(Sorry for deleting my previous question. After I found the solution it just looked so much better that I didn't think I needed to ask it anymore, and I deleted it before seeing your answer, however I managed to do that.) Anyway, I hope it looks fine now!
Screenshot from 2022-01-31 22-57-13

Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

nice, almost done =)

evap/contributor/forms.py Outdated Show resolved Hide resolved
evap/staff/forms.py Outdated Show resolved Hide resolved
Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

select2 was updated to 4.1.0-rc1 on upstream:main in between (see #1702) -- so after merging or rebasing, the select2 stuff should disappear from the diff.

evap/contributor/tests/test_forms.py Outdated Show resolved Hide resolved
evap/contributor/tests/test_views.py Outdated Show resolved Hide resolved
evap/staff/tests/test_forms.py Outdated Show resolved Hide resolved
evap/static/scss/_adjustments.scss Show resolved Hide resolved
Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

Code is fine with me, haven't tested manually.

evap/contributor/tests/test_views.py Outdated Show resolved Hide resolved
evap/contributor/tests/test_views.py Outdated Show resolved Hide resolved
@knollengewaechs knollengewaechs requested a review from janno42 March 21, 2022 19:48
Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Code looks good, UI does too as far as I can tell - @janno42 I am relying on your thorough review though :)

@knollengewaechs knollengewaechs force-pushed the participants_editable_by_editors branch from 9645013 to ebd6a50 Compare March 21, 2022 21:03
@janno42 janno42 merged commit 0d25cf8 into e-valuation:main Mar 25, 2022
@janno42
Copy link
Member

janno42 commented Mar 25, 2022

Thank you so much 👍

@knollengewaechs
Copy link
Collaborator Author

yeeeey, this PR finally made it! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Make participants editable by editors
4 participants