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

Feature/review and secretary pages redesign #688

Merged
merged 24 commits into from
Sep 23, 2024

Conversation

EdoStorm96
Copy link
Contributor

So, here I've restyled a lot of the remaining (form) pages. Mostly relating to the review process and secretary side. This is mostly self explanatory, however, I did make some minor design changes:

  • I've renamed to stepper block on fetc_form_base to sidebar, as it can be used to accomodate other things, like the review detail sidebar.
  • I've simplified the buttons block, to make overriding buttons a lot easier.
  • I was running into bugs with review_actions, so I've updated the logic. See b702725. To me, it seems that this is how it should work. Please pay close attention here.

I am unsure if there are pages leftover that still need a redesign. Let me know if you can think of any.

@EdoStorm96 EdoStorm96 requested a review from miggol August 28, 2024 10:22
@EdoStorm96 EdoStorm96 force-pushed the feature/review_and_secretary_pages_redesign branch from 24799cb to 4c29c77 Compare August 29, 2024 08:10
Copy link
Contributor

@miggol miggol left a comment

Choose a reason for hiding this comment

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

Good progress on the redesign.

  • I've renamed to stepper block on fetc_form_base to sidebar, as it can be used to accomodate other things, like the review detail sidebar.
  • I've simplified the buttons block, to make overriding buttons a lot easier.

These are excellent changes

@@ -1,4 +1,4 @@
{% extends "base/fetc_base.html" %}
div{% extends "base/fetc_form_base.html" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This div ain't right

Also changing the base has removed the sidebar from this view. Or at least I think this is the change that did that. Could you reinsert the review_detail_sidebar?

We could have an intermediate fetc_review_base.html that just inserts that sidebar by default. But also kind of overkill

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, I checked it and it seems there was never a sidebar on this page. It just feels like it's missing because of the new empty space.

I think it would be nice if the review detail sidebar was on this page, like with the discontinue form. Makes it easier to confirm you're dealing with the right proposal.

@@ -85,7 +85,7 @@ def get_available_decision(self):
user = self.user
review = self.review

if review.stage in (review.Stages.COMMISSION, review.Stages.SUPERVISOR):
if review.stage not in (Review.Stages.COMMISSION, Review.Stages.SUPERVISOR):
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is correct I think!

Review.Stages.CLOSING,
Review.Stages.CLOSED,
Copy link
Contributor

Choose a reason for hiding this comment

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

The blame history of this logic is a mess, lol. How did continuations get in here? This should have been caught by someone.

Well, it's been caught now at least. Good find.

In my opinion, this action should be available to the secretary in the following stages:

  • Assignment (obviously)
  • Commission
  • Closing
    • This stage is automatically reached when all opinions have been gathered. But it happens often enough that the secretary wants to add another reviewer before making the final call, so this action should still be available in that stage.

As it stands now, this action will be available in the supervisor stage, which I don't think is right. Could result in weird behaviour if the secretary accidentally adds or removes someone in that stage.

@EdoStorm96 EdoStorm96 requested a review from miggol September 17, 2024 07:56
Copy link
Contributor

@miggol miggol left a comment

Choose a reason for hiding this comment

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

Just one template issue but otherwise all seems well

{% block form-buttons %}
{% include "base/form_buttons.html" %}
{% endblock %}
{% block form-buttons %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think something went wrong when merging. This template now has two {% block form-buttons %} (see below, line 42) which causes an error.

@EdoStorm96 EdoStorm96 force-pushed the feature/review_and_secretary_pages_redesign branch from 287118b to 97c953a Compare September 18, 2024 11:32
Copy link
Contributor

@miggol miggol left a comment

Choose a reason for hiding this comment

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

Looks good now

@EdoStorm96 EdoStorm96 merged commit a692b96 into major/4 Sep 23, 2024
3 checks passed
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.

2 participants