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

Settings page added #133

Merged
merged 15 commits into from
Jul 19, 2019
Merged

Settings page added #133

merged 15 commits into from
Jul 19, 2019

Conversation

Nppatel97
Copy link
Collaborator

@Nppatel97 Nppatel97 commented Jun 7, 2019

New Settings page made. @AhmedNSidd if you can add a link or something for this so I can view this page while modifying, that would be dope! Also, there are only two settings that I am aware of right? To honor or not honor availability and ability to reschedule. Also, this is literally just a copy pasta code of agenda.html so I haven't changed anything except the title which says 'Settings | Schedulearn'.

Copy link
Owner

@sdevalapurkar sdevalapurkar left a 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, but I wonder if there is any way to share code/reuse components from existing files instead of copy 🍝 code... any thoughts?

@Nppatel97
Copy link
Collaborator Author

Looks good to me, but I wonder if there is any way to share code/reuse components from existing files instead of copy 🍝 code... any thoughts?

That's a great idea! We can make a skeleton file and reuse it in all other files but it would be kinda difficult to do in HTML.. Something like React would be awesome for that. Or if there is some feature in Python or JS that I may not be aware of

@AhmedNSidd
Copy link
Collaborator

AhmedNSidd commented Jun 12, 2019

I am actually working on a branch right now that's doing something similar to what you're asking @sdevalapurkar. Specifically, it's setting up css files that are generic to all pages to minimize reuse of code. It's taking some time to work on it because there's a lot of variability across the css files. In the future, we can also work on creating base html files that contain maybe only the navbar and the other html files can extend that base html. Anyway @Nppatel97, I pushed some code up that has set up the routing for the settings page. So you can see the page now at http://localhost:8000/dashboard/settings/
If you have any questions lmk

EDIT: also yes, there's only two options that I need you to add right now #120 and #123

@AhmedNSidd AhmedNSidd changed the title settings page added [WIP] settings page added Jun 12, 2019
@Nppatel97
Copy link
Collaborator Author

Alright guys, Settings page is complete with all the links and options in place. My Profile link is moved to primary navbar and is replaced by settings. Will have to do this in all other files though. Now just need to add the back-end logic to this. Here are some images. Let me know what you think and any changes that it might need @AhmedNSidd @sdevalapurkar @DSchriemer :)

settings-1
settings-2

@AhmedNSidd
Copy link
Collaborator

That looks absolutely fantastic man. Exactly how I wanted it to look. GREAT job. Although for the setting titles, I think we should capitalize the first letter of each word. After work, I'm gonna get home and check out how this looks in the browser and start implementing some of the backend into this

@@ -0,0 +1,11 @@
$(document).ready(function () {
Copy link
Owner

Choose a reason for hiding this comment

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

can we use ES6 style arrow functions here?

$(document).ready(() => {

});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For sure!

schedule_lessons/dashboard/urls.py Outdated Show resolved Hide resolved
notifications = Notification.objects.filter(
user=request.user).order_by("-created_on")
context = {
"notifications": []
Copy link
Owner

Choose a reason for hiding this comment

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

question: how come context doesn't have a property called unread_notifications even tho it is being used later on?

Copy link
Collaborator

Choose a reason for hiding this comment

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

True actually, I opened up issue #139 about this so we can clean this up later since there are other instances where I do this in this file.

Copy link
Collaborator

@AhmedNSidd AhmedNSidd left a comment

Choose a reason for hiding this comment

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

I had a chance to look at this after work. I have a few comments on things that can be improved a bit. Firstly I think the Settings Title in the webpage should be bigger (maybe make it h3 or even bigger, use your best judgement). Make the setting names and descriptions relatively bigger too (idk if you made them small so it looks better on mobile, if that's the case, let me know).

I think for the future of the settings page, I wanna move the "Blocked Users", "Change Password", and "Delete Account" options from the profile page to the settings page. It's not something I think we should do in this PR but maybe we should setup issues to tackle these issues in future versions (or if we get the chance, v1.0).

@DSchriemer
Copy link
Collaborator

Looking very good, nice work!!

@Nppatel97
Copy link
Collaborator Author

I had a chance to look at this after work. I have a few comments on things that can be improved a bit. Firstly I think the Settings Title in the webpage should be bigger (maybe make it h3 or even bigger, use your best judgement). Make the setting names and descriptions relatively bigger too (idk if you made them small so it looks better on mobile, if that's the case, let me know).

I think for the future of the settings page, I wanna move the "Blocked Users", "Change Password", and "Delete Account" options from the profile page to the settings page. It's not something I think we should do in this PR but maybe we should setup issues to tackle these issues in future versions (or if we get the chance, v1.0).

Yeah I wanted to make them look consistent with the text in navbars so I chose these sizes for Settings title and options. I'll fiddle around with sizes and see which ones look better on both mobile and desktop.

And yes, I think those options would be better in settings (than in my profile page), I think it could be easy to do it in v1.0 as long as there is not much back-end logic involved. (Maybe even in this PR?)

@Nppatel97
Copy link
Collaborator Author

Looking very good, nice work!!

Thanks! Let me know if you have any ideas on improving this UI/UX :)

@Nppatel97
Copy link
Collaborator Author

Nppatel97 commented Jun 29, 2019

I have updated all the navbar links so now they all show my profile in primary navbar and settings under profile pic. Also moved the change password, blocked users and delete account options to the settings page. Not sure how it connects to the back-end so I am leaving that for @AhmedNSidd.

image

Also, the secondary navbar of public_profile.html has not been optimized by me in past because there are a lot (a lotttt) of if-else django stuff in it as shown in the image ↓

image

Check the lastest commit and let me know of any bugs or if I missed anything. Cheers! 🍻

EDIT: Ooo and about making the headers a little bigger, I tried to fiddle around with different sizes and it looked a little inconsistent and odd so I kept it the way it was.

@AhmedNSidd
Copy link
Collaborator

AhmedNSidd commented Jun 29, 2019

@Nppatel97 Please check out this latest commit and make sure it looks alright. I used the chrome coverage tool to get rid of any unused css/js code for the settings page.

EDIT: Also thanks for moving the buttons to the settings page, they look good. However, in a future commit, I wanna move them down and maybe remove the extended navbar. It looks good for now, I'll just implement the backend logic now, it should be easy enough to move them down later.

/* END Navbar for logged in Users */

#picturePrompt {
height: 300px;
width: 90%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am guessing you made this change to look better on mobile, but the image was too huge on laptop. Please check out my commit on "Fixing the huge picture prompt" and let me know if the changes I made look alright

Copy link
Owner

@sdevalapurkar sdevalapurkar left a comment

Choose a reason for hiding this comment

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

nice work, some stuff to clean up a bit and some small nits/suggestions but overall looks good

schedule_lessons/dashboard/static/dashboard/js/settings.js Outdated Show resolved Hide resolved
schedule_lessons/dashboard/static/dashboard/js/settings.js Outdated Show resolved Hide resolved
schedule_lessons/dashboard/static/dashboard/js/settings.js Outdated Show resolved Hide resolved
schedule_lessons/dashboard/static/dashboard/js/settings.js Outdated Show resolved Hide resolved
schedule_lessons/dashboard/static/dashboard/js/settings.js Outdated Show resolved Hide resolved
schedule_lessons/dashboard/static/dashboard/js/settings.js Outdated Show resolved Hide resolved
schedule_lessons/dashboard/static/dashboard/js/settings.js Outdated Show resolved Hide resolved

$.ajaxSetup({
beforeSend: function (xhr, settings) {
if (!(/^http:.*/.test(settings.url) || /^https:.*/.test(settings.url))) {
Copy link
Owner

Choose a reason for hiding this comment

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

not sure what is being checked in this if statement, could you please clarify, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I changed the if condition, let me know if the new one makes sense to you.

Copy link
Owner

@sdevalapurkar sdevalapurkar left a comment

Choose a reason for hiding this comment

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

the js changes LGTM, one minor comment

schedule_lessons/dashboard/static/dashboard/js/settings.js Outdated Show resolved Hide resolved
@AhmedNSidd AhmedNSidd changed the title [WIP] settings page added Settings page added Jul 18, 2019
Copy link
Owner

@sdevalapurkar sdevalapurkar left a comment

Choose a reason for hiding this comment

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

js tips and tricks

@@ -55,6 +55,15 @@ $(document).ready(() => {
});
}
});

$(".preference").click((event) => {
const preference_id = event.target.id;
Copy link
Owner

Choose a reason for hiding this comment

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

since we are only using a few properties of this event object, let's try this:

const { id, checked } = event.target;

and then use id and checked

const preference_id = event.target.id;
$.ajax({
type: "POST",
url: "/dashboard/modify_preference/" + preference_id + "/",
Copy link
Owner

Choose a reason for hiding this comment

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

url: /dashboard/modify_preference/${id}/

Copy link
Owner

Choose a reason for hiding this comment

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

use the string template literal syntax

@AhmedNSidd AhmedNSidd merged commit 3a91ab8 into master Jul 19, 2019
AhmedNSidd added a commit that referenced this pull request Jul 30, 2019
AhmedNSidd added a commit that referenced this pull request Aug 3, 2019
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.

None yet

4 participants