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

new branch created change added to make a contactus py form #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

purplephoenix92
Copy link

Made a contact us . py form and created a branch to commit possible changes. I wanted to make sure that I was following the guide that you referenced in get started guide and using the working with forms. The next thing that I was going to do is install docker in a round a bout way and then add the necessary pieces in the views.py next hopefully. I'm not for sure if it is the app.js that you want to possibly add the piece that was missing there, the async did mount (?)

@purplephoenix92 purplephoenix92 requested a review from a team as a code owner April 12, 2020 04:31
@@ -0,0 +1,9 @@
rom django import forms
Copy link
Member

@bretwalker bretwalker Apr 12, 2020

Choose a reason for hiding this comment

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

This will need to be from django.... Small typo. But I think it'd be better to move your form class to another place in the codebase. See the comment I left on the main PR for more info.

@bretwalker
Copy link
Member

Thanks, I'm glad you're contributing!

I think the best place for this class is in the existing forms module (forms.py) in the units app (the same directory where you created contactus.py.

The units app is the core of the app and has some of the other catch-all things like the IndexView and GettingStartedView; it's not solely dedicated to functionality around rental units.

Once you're ready to start working in on the view, you can add it to the existing views module (views.py) in the units app.

I think we can have it simply email the "managers" of the site. Django provides a nice mail_managers method to do this: https://docs.djangoproject.com/en/3.0/topics/email/#mail-managers

You'll also need a simple template that renders the form. You can mimic something like this, which inherits the base template and renders the form as a table: https://github.com/codeforkyana/renters-rights/blob/master/renters_rights/noauth/templates/log-in.html.

I'm not for sure if it is the app.js that you want to possibly add the piece that was missing there, the async did mount (?)
The JS that I'd like to convert from Promises to async is all in unit_add_image.html, and it's probably best to handle that as a separate chunk of work.

Let me know if you have any questions or need any help!

@bretwalker bretwalker linked an issue Apr 12, 2020 that may be closed by this pull request
Copy link
Member

@bretwalker bretwalker left a comment

Choose a reason for hiding this comment

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

Great work so far!
There are some Django-specific changes we should make so the form can accept a large comment and support translation.

class NameForm(forms.form):
your_email = forms.CharField(label = 'Your Email', max_length = 100)
your_name = forms.CharField(label = 'Your Name', max_length = 100)
your_comments = forms.CharField(label = 'Your Comments', max_length = 100)
Copy link
Member

Choose a reason for hiding this comment

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

You'll want to add widget=forms.Textarea to this field so it renders as a multiline text area.
And let's make the max_length 10,000.

rom django import forms

class NameForm(forms.form):
your_email = forms.CharField(label = 'Your Email', max_length = 100)
Copy link
Member

Choose a reason for hiding this comment

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

One small change to your labels. We want these to be able to be translated into other languages easily, and Django facilitates that through their translation module.

In the forms module, you'll see a line like from django.utils.translation import gettext_lazy as _. And then strings wrapped in _(). This makes them easy to export for translation and translatable at runtime.

So for your labels, could you wrap them in _()? For example, label='Your Name would become _('Your name').

When you move this form class to the forms module, _ will have been imported for you, so you won't need to import it.

@purplephoenix92
Copy link
Author

purplephoenix92 commented Apr 19, 2020 via email

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.

Add a contact page
2 participants