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

Added thematic toggle and preferences page #51

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Icyviolet23
Copy link

@Icyviolet23 Icyviolet23 commented Dec 8, 2023

Resolves #37

Major changes

  1. Added Preferences Model
  2. Added one to one relation between the Preferences Model and the Abstract User model. Each User will have their own preferences.
  3. Added Preferences ModelForm which will be used to collect user preferences
  4. Added view to handle Preferences Form POST request, create an instance of the Preferences model and save it in the DB
  5. Created HTML Template for preferences page
  6. Edited HTML template for navigation.html to add navigation to preferences page
  7. Added dark/light mode button and corresponding light/dark mode toggle functionality

Refer to the following DEMO:
https://www.youtube.com/watch?v=0qfaXa9EYyA&ab_channel=TzeHngLoke

Icyviolet23 and others added 7 commits November 28, 2023 21:52
…tch preferences and display them according. Added model form to allow users to customize their own preferences dynamically
…er preferences. The mode will be automatically set to the correpsonding preferred mode
Copy link

codeclimate bot commented Dec 8, 2023

Code Climate has analyzed commit 122e4fb and detected 41 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2
Style 39

View more on Code Climate.

Copy link
Member

@brylie brylie left a comment

Choose a reason for hiding this comment

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

Overall, this is looking great! I've just added a few comments about internationalization tags so our app can be localized in different languages.

https://docs.djangoproject.com/en/5.0/topics/i18n/translation/#translation

<div class="card-body py-5 px-md-5">
<div class="row d-flex justify-content-center ">
<div class="col-lg-8">
<h2 class="fw-bold" id = "current-preferences">Your Current Preferences</h2>
Copy link
Member

Choose a reason for hiding this comment

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

Remember to add template internationalization so we can translate this string into the user language.

<h2 class="fw-bold" id = "current-preferences">Your Current Preferences</h2>
<div class="col-lg-15">
{% if not request.user.preferences %}
<h3 class="fw" id = "no-preferences">No current preferences. Set some!</h3>
Copy link
Member

Choose a reason for hiding this comment

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

Please add i18n tag ´translate' here.

<div class="card-body py-5 px-md-5">
<div class="row d-flex justify-content-center ">
<div class="col-lg-8">
<h2 class="fw-bold " id = "set-preference-message">Set Preferences</h2>
Copy link
Member

Choose a reason for hiding this comment

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

Remember i18n ´translate'tag here.

{{form|crispy}}
</table>
{% csrf_token %}
<button class="btn btn-primary " id = "submit_preferences_button" type="submit">Submit</button>
Copy link
Member

Choose a reason for hiding this comment

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

I18n tag here

'Language': forms.Select(attrs={'class': 'form-control'}),
'Mode': forms.Select(attrs={'class': 'form-control'}),
}
labels = {'Language': 'Preferred Language',
Copy link
Member

Choose a reason for hiding this comment

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

Remember i18n tag for these labels using gettext. E.g. _("Preferred Language")

preferences/models.py Outdated Show resolved Hide resolved
templates/base.html Outdated Show resolved Hide resolved
templates/base.html Outdated Show resolved Hide resolved
# null check for preferences field
if request.user.preferences:
fields = [(field.name, field.value_to_string(request.user.preferences)) for field in Preferences._meta.fields]
context['fields'] = fields
Copy link
Member

Choose a reason for hiding this comment

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

I think we only need to return a form instance pre-populated with the existing user preferences.Django takes care of everything else.

Suggested change
context['fields'] = fields
context['form'] = PreferencesForm(instance=request.user.preferences)

context['form'] = PreferencesForm ()
# null check for preferences field
if request.user.preferences:
fields = [(field.name, field.value_to_string(request.user.preferences)) for field in Preferences._meta.fields]
Copy link
Member

Choose a reason for hiding this comment

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

This is probably unnecessary

Suggested change
fields = [(field.name, field.value_to_string(request.user.preferences)) for field in Preferences._meta.fields]

class PreferencesForm (ModelForm):
class Meta:
model = Preferences
fields = ('Language', 'Mode')
Copy link
Member

Choose a reason for hiding this comment

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

These fields should match the model fields

Suggested change
fields = ('Language', 'Mode')
fields = ('language', 'color_mode')

from django.conf import settings

# for user preferences
class Preferences (models.Model):
Copy link
Member

Choose a reason for hiding this comment

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

I believe adding a ‘user = models.OneToOne(…)’ field on the ‘Preferences’ model will make the view.py code simpler.

@@ -10,3 +11,8 @@ class Meta:

def __str__(self):
return self.username

#store user preferences in a one to one mapping
preferences = models.OneToOneField(Preferences, on_delete=models.CASCADE, blank=True, null=True)
Copy link
Member

Choose a reason for hiding this comment

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

Let’s define the recon the Preferences model rather than the user. Then, use a ‘reverse_name=“preferences”’ on the ‘user’ OneToOne field attached to the Preferences model. That way, we assign the user to the Preferences instance directly in view.py.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
preferences = models.OneToOneField(Preferences, on_delete=models.CASCADE, blank=True, null=True)

Mode = form.cleaned_data['Mode'],
)

preferences.save ()
Copy link
Member

Choose a reason for hiding this comment

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

This can just be ‘form.save()’, so there is no need to create a temporary Preferences instance above

Suggested change
preferences.save ()
preferences = form.save()

Copy link
Member

@brylie brylie left a comment

Choose a reason for hiding this comment

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

It looks like there are some conflicts in settings.py. Let's resolve those conflicts so we can merge this PR soon. Overall, the PR is looking good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Add a dark/light mode toggle switch on navbar
3 participants