-
Notifications
You must be signed in to change notification settings - Fork 5
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
Remove nominations page #56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally good, but should address my feedback before merging.
Will do the changes but, I see you have used head tag but, head is not imported in the code. I will do a Pr fixing that |
Co-authored-by: Nicholas Langford <[email protected]>
Thanks, nice catch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I'll wait for @lhvy before merging though
Was considering utilising inline conditional rendering (e.g. ternary) to avoid repeated code like the head/title, and the fluff around the h1, but it ended up being too convoluted. I think how it is currently is the best solution, apart from a complete rewrite abstracting sections into different components like NomineeSection and NominationInfo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meant to request @lhvy rather than re-request review from myself, oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Remember to add Resolves #xx
to your PR description so the associated issue will close automatically.
Resolves #52
Changes -
Highlights -