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

#S06/#S09 As Staff I would like to enroll/withdraw as a Judge #90

Closed
wants to merge 13 commits into from

Conversation

Ri-Dearg
Copy link
Contributor

Description

Pull request type

  • #M01 (Miscellaneous User Story Initial README setup #1)
  • #P02 (Participant User Story Fix Color Palette on README #2)
  • #S06 - As Staff I would like to enroll as a Judge
  • #S09 - As Staff I would like to withdraw as a Judge
  • #A04 (Admin User Story Update README with Wiki links #4)
  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Related Issue

#35
#38

Testing

  • Test on all screen sizes checked.
  • Have written thorough tests for both views.
  • Ran a series of manual Javascript tests.

Screenshots

Logged In as Staff
Screenshot 2020-10-21 at 16 20 05
Screenshot 2020-10-21 at 16 17 41

Not Staff
Screenshot 2020-10-21 at 16 18 32

Additional Information

  • messages.html and base.html have been edited to accommodate the toast pop-ups. I have pulled the most recent version to make sure I am not changing anything already present.
  • A few classes to position the toasts were added to style.css
  • If the Fetch form is unnecessary, I can switch it to a simpler version with page reload easily. I thought it might be a handy template for other button-type forms, such as joining a hackathon, team, etc.

Does this introduce a breaking change

  • Yes
  • No

Added basic view and connected it to ListView.
Includes logic to display judges and the option to enroll.
Does not process any data yet, simply proof of concept.
Merge with original fork
Toast popup function is reformatted. Fetch now catches errors
The first "Unenroll" click wouldn't change because of whitespace.
@Ri-Dearg Ri-Dearg marked this pull request as ready for review October 21, 2020 14:39
Copy link
Member

@stefdworschak stefdworschak left a comment

Choose a reason for hiding this comment

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

@Ri-Dearg overall a very nice change. Just consider moving from AJAX to just submitting the form; unless there is a specific reason for it that I missed.

{% block js %}
{{ block.super }}
{% if request.user.is_staff %}
<script>
Copy link
Member

Choose a reason for hiding this comment

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

Could you extract the JS content to an external file please?

Copy link
Member

Choose a reason for hiding this comment

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

Also, why have you gone with an AJAX approach here instead of just submitting the form and redirecting back to the updated page? Since we are using Django I find this a bit counter intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, cool. I'll outline my AJAX reasoning below and you make the call.

Will I move it to the main script.js or will I create a different one? I wasn't sure where it would be more appropriate so I used the block js.

I went with AJAX because it's easy and fast. Some buttons do pretty much the same thing, only adding/removing info from the database, so it's simpler. In other user stories, like "enroll/withdraw as a participant", it would be easy to add another button on the page that would function similarly, using the same scripts. Some users would have two buttons. This way it's more like clickable options in front of you, instead of sending numerous forms. I think it's nice.

From a user perspective, it's a smoother, more responsive experience. The less required page loading, the better, no? A page-load makes sense for detailed forms or new content, but it's a small update and doesn't provide the user with any new content or info. So, I thought "why reload the whole page?". I got a lot of feedback on my final milestone about users not wanting to stay on sites with page-reloads that don't provide new content or engagement and I ended up using a lot of ajax. In summary, I thought it was a useful addition for UX.

I'm wondering why is it counter-intuitive with Django?
If you think it's better without it, I'll take it out, still, it would be useful for me to learn.

Also, I've realised I need to add code to prevent users from signing up to Hackathons past their end date, so I have to do that too before I've finished.

Copy link
Member

Choose a reason for hiding this comment

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

@Ri-Dearg okay fair enough. It is a bit counter intuitive for me, but I can see your point and thinke we can keep the AJAX logic then. In terms of the external JS. I'd just add it to script.js since we don't really have any JS at the moment it should be fine.

Copy link
Member

@stefdworschak stefdworschak left a comment

Choose a reason for hiding this comment

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

LGTM

Just split out the JS to an external file as discussed and add in the logic to prevent users from signing up to Hackathons once they're over and we're good to merge it.

@stefdworschak
Copy link
Member

@Ri-Dearg I think there are a few merge conflicts now. Please resolve them and revert back to me so I can merge this before it goes stale. Thanks!

@Ri-Dearg
Copy link
Contributor Author

Ri-Dearg commented Oct 27, 2020

@stefdworschak

Hi Stefan, sorry, I haven't been able to work on this. I've gotten quite sick with flu symptoms and haven't been able to concentrate. I'll get back to it as soon as I'm able.

@stefdworschak
Copy link
Member

@Ri-Dearg no problems at all! Your health is much more important. Please take care and I hope you feel better soon!

@Ri-Dearg
Copy link
Contributor Author

Ri-Dearg commented Nov 5, 2020

Hi @stefdworschak,
I think it should be okay to close this pull request. Sorry, I wasn't able to finish it up. @mkuti and I had a good conversation about the code and the necessary changes. We agreed that she would integrate and continue the work. It looks like it's been done in issue #113.

If you think it would be better that I tidy it up and push anyway, I can get that done tomorrow.

@TravelTimN
Copy link
Collaborator

Hi @stefdworschak,
I think it should be okay to close this pull request. Sorry, I wasn't able to finish it up. @mkuti and I had a good conversation about the code and the necessary changes. We agreed that she would integrate and continue the work. It looks like it's been done in issue #113.

If you think it would be better that I tidy it up and push anyway, I can get that done tomorrow.

@stefdworschak
What's your thought here?
They're quite similar, do we need anything additional from the PR #90 ?

@TravelTimN
Copy link
Collaborator

Closing PR as covered by #113

@TravelTimN TravelTimN closed this Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted PR during Hacktoberfest
Projects
None yet
3 participants