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

Feat/supervisor decide on detail view #649

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

EdoStorm96
Copy link
Contributor

Fixes #612. There is no supervisor decide button on the review detail page. Granted this is a super niche edge case, where someone, has to be both a member of a chamber AND the supervisor, but I guess this did occur. This should fix the issue.

I also noticed that the text "Uitleg secretaris" on action_explanation.html, was outside the if-statement which checks if a user is a secretary, so this always rendered, which I fixed too. I didn´t really feel this warranted creating an issue, so I just pushed this here. I will make an issue for documentation, if you would prefer.

@EdoStorm96 EdoStorm96 requested review from tymees and miggol April 3, 2024 13:14
Copy link
Member

@tymees tymees left a comment

Choose a reason for hiding this comment

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

Simple enough. I did have one small nitpick, but I'm approving it anyway :)

I will make an issue for documentation, if you would prefer.

Ehhh, for something this minor it's not worth the hassle imo. In the future you can consider writing a commit body text[1] explaining why this change was made, as that will show up in git-blame as well. (Altough, even that would not be strictly necessary imo).

[1] A longer text in your commit message, after the 'title' (with a spacer line in-between). Once again, I'd like to plug https://www.conventionalcommits.org/en/v1.0.0/

<ul>
{% if request.user|is_secretary %}
{% if request.user|is_secretary %}
<div class="col-12 {% if request.user|is_secretary %}col-md-6{% endif %}">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<div class="col-12 {% if request.user|is_secretary %}col-md-6{% endif %}">
<div class="col-12 col-md-6">

The whole thing is scoped under that if now, so it will always be true. Might as well remove the if statement, as it's kinda ugly ;)

@EdoStorm96 EdoStorm96 merged commit 6275201 into develop Apr 9, 2024
4 checks passed
@EdoStorm96 EdoStorm96 deleted the feat/supervisor_decide_on_detail_view branch April 9, 2024 14:36
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.

Decide button is missing from supervisor decision
2 participants