-
Notifications
You must be signed in to change notification settings - Fork 79
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
Contact Form Feedback as Modal Issue#357 #422
base: main
Are you sure you want to change the base?
Conversation
Won't have time to look at the code for a bit, but I wanted to call out that this is quite a wonderful PR (description and demo). A great model for anyone opening PRs...on any project. |
Ummm please disregard the "close" and "reopen" that just happened... I accidentally opened this page with the mobile app, and definitely don't know how to use the mobile app properly. Whoops! |
After meeting yesterday, here are some notes that will be addressed:
EDIT:
|
Progress UpdateI've addressed bullet_points[0:3] above; I created a new little branch and opened an explanatory PR against my own fork of the repo here because I don't want to clutter up this PR's commit history (as I expect to make further improvements). TBH I'm new to PR commit etiquette so I'm just trying not to make a mess while I'm tinkering around 😆 TL;DR - I fixed what I broke in /templates/slim.html, and the message-box + modal behaviors are now living together in peace. I did this by adding a I am working on another branch where the modal functionality is more self-contained instead of copy/pasted, and should be reusable in any view where it may be desirable. No writeup/PR yet, however the work is visible on my fork on the "modularize-modal" branch. |
You are fine to update this PR anytime you want, no worries there. But do whatever your workflow requires. I added the label for in progress. Remove that when you are all done, and feel free to ping folks too! |
Hello again! My bad for such a delay. I think I'm just about done; the modal behavior is fairly reusable now (and no longer breaks any existing behavior 😇). Just as I was about to walk though and write out the updates, I somehow managed to break my local dev environment...? I'm not exactly sure what happened, but my docker containers are starting with different names than before and I'm getting Django errors saying that certain data doesn't exist (I think both session data and db stuff). Anyway, I'll be back with once I figure out what's going on. I'm 99.99% sure it's not a code problem, b/c even previously-stable branches are having this issue. Cheers! |
bcf2d76
to
3f6a90c
Compare
Okay! Back in business. I just needed to reset the dev-db and remake the data. At this point in time, I think I'm just about done (P.S. dangit I might need to adjust the img and its css); I can make any changes you like, but I think this has potential to be the PR's final state. TL;DR:
"omg I love reading complete changelogs":Here is what has changed vs chipy.org/main: chipy_org/apps/main/templates/main/homepage.html AND slim.htmlThese file changes are identical, because every page extends slim except for the homepage; so this code lives in both spots. Previously:
{% if messages %}
<div class="container-xl">
<div class="row">
<div class="col">
{% include "_messages.html" %}
</div>
</div>
</div>
{% endif %} Now:
chipy_org/apps/contact/views.pyThere are two changes of interest to this file, and they are related to:
(This is how you tell any view to use the modal behavior!) Config
modal_config = {
"close_button_redirect": "/",
"close_button_label": "Return to home"
} The first key represents the page where it will redirect the user upon clicking the button to close the modal. The second key is the text that should be used to label the close button. Passing that config from the View to the TemplateAs with any variables to be used in the template, we'll use Django context (a dict). The Contact view wasn't already sharing any data with the templates, so I added the method to the view. In some other views, the method is already present; you'll just need to The method looks like this: def get_context_data(self, **kwargs):
""" Used to access message_as_modal in template as context """
context = super(ContactView, self).get_context_data(**kwargs)
context.update({"message_as_modal": self.message_as_modal})
context.update({"modal_config": self.modal_config})
return context Now within any templates, we can access this data as chipy_org/templates/_modal.htmlIt may seem that we are using either "_messages.html" or "_modal.html", but this is not the case. In short, "_modal.html" is just a pretty wrapper for the "_messages.html" template within it. "_messages.html" is the only file that handles/empties the message queue. "_modal.html" has three parts:
chipy_org/templates/_messages.htmlThis file serves the message we've all been waiting for! It has one simple conditional:
chipy_org/static/css/shiny.cssAdded a few styling rules to make the modal elements align correctly. |
@spark-c this looks really great. I will review this at the upcoming project night Thursday, November 18th. Thanks for your efforts. |
Hello!
This PR is an in-progress implementation of Issue #357.
I think my implementation may be a little hacky, and I'm definitely open to an alternative solution! (I wanted to give it a go alone first to see if I could do it 😃 ). I'll explain my approach below.
P.S. I write long... my bad. I went into detail b/c I wanted to make sure I understood / to talk myself through everything once more.
Before the wall of text below, here are my questions:
How do you feel about the implementation?
Where should the form and/or modal redirect?
Demo:
chipy.contact.mp4
What was the previous functionality?
Previously, submitting the contact form resulted in a redirect to the chipy.org homepage, where a success/fail message would be presented like so:
Redirect
In its current state, this PR updates chipy_org/apps/contact/views.py such that the form submission redirects to the /contact page instead of going back to the homepage. It felt a little surprising to me when I was taken back to home by the "Send" button without warning.
If you'd like to preserve the old behavior, I think it might feel good if:
How was the modal HTML built?
I found an existing modal implementation in use for the site's login functionality; the HTML is located in shiny/navbar.html, and the javascript for its behavior is found in static/js/pinax.modal.js (neither file has been modified).
Here is where I begin to get concerned about being a bit hacky -- I copy/pasted the HTML for the Login Modal into the apps/contact/templates/contact.html file, and made appropriate adjustments to its structure/data and classes.
How was the modal behavior built?
I continued to use the
django.contrib.messages
framework that already existed:What triggers the modal to show?
In the original code (pinax.modal.js), the modal is set up to be triggered by clicking an element (adding bootstrap classes to an element that makes it trigger the modal functionality).
Since this feedback should be automatic and not the result of user clicks, I had a little workaround:
display:none
hardcoded into the tag