-
-
Notifications
You must be signed in to change notification settings - Fork 61
Reworking contributions show respond #851
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
Conversation
fde664f to
46d8b64
Compare
solebared
left a comment
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.
Gave it first pass and it looks great! 👏🏾 .
I didn't dig into implementations of the extracted partials, assuming they were mostly copied over.
Couple comments inline.
4c1c592 to
a04a873
Compare
solebared
left a comment
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.
Woohoo!! 🙌🏾
cf24ca2 to
25a762b
Compare
Why
In an effort to make things more RESTful where it is simple to do so, this PR combines the show and respond views into show and then uses pundit to differentiate between the views. There are also quite a few places where I then either removed the "Respond" button if both existed, or left it as "Respond" but changed the path.
Outstanding issues/things I could use some help with:
Pre-Merge Checklist
What
Initial Respond Page

Both are now the show but it depends on whether you are logged in or not, and if you are an admin or not


How
Testing
Next Steps
This is part of #840 (Re-enabling registration)
Outstanding Questions, Concerns and Other Notes
Accessibility
Security
Meta