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

Profilepage #88

Conversation

spline2hg
Copy link

I have added the profile page using a Profile model and corersponding form, currently this has 2 different profile page,that we can access

 path('profile/', auth_views.profile, name='profile'),
 path('profile_a/', auth_views.profile_alpine, name='profile')

The one available at profile/ uses a is_editing variable to switch between edit and view mode for profile and the one at profile_a uses alpine js for this, there is also diff html files for this, i have tried to fix the issues i faced ,and it seems it works fine other than the few ui changes required, Plz review the PR and let me know there are any changes needed or areas of improvement ,feedback is greatly appreciated, thanks for guiding and letting me work on this

@nfoert nfoert changed the base branch from main to development October 9, 2024 21:35
@nfoert
Copy link
Owner

nfoert commented Oct 9, 2024

Thanks for working on this @spline2hg! This looks great
A few things I've noticed:

  • Can the profile picture be automatically downscaled to a certain size limit/maximum image size?
  • This should eventually be moved to just be one single page, would you like assistance converting it all to use Alpine.js?

Additionally I worked to adjust most of the styles to fit the current theme of the site, I also opened #89, issue #84 will likely adjust how the styles are done anyway

Additionally there's a lot of open issues and discussion around the profile and authentication system (#32, #46, #81, #42), some features (eg. changing password) might need to wait until these are implemented, but perhaps this could still be merged into development or a separate branch for managing the new profile and authentication system (to allow you to make sure you still get your hacktoberfest credit in time)

@spline2hg
Copy link
Author

Thanks for working on this @spline2hg! This looks great A few things I've noticed:

Yep, Thanks

* Can the profile picture be automatically downscaled to a certain size limit/maximum image size?

So something like compression?

* This should eventually be moved to just be one single page, would you like assistance converting it all to use Alpine.js?

i have made the alpine js version too, u could access it a profile_a, or did u mean anything, i didnt get it

Additionally I worked to adjust most of the styles to fit the current theme of the site, I also opened #89, issue #84 will likely adjust how the styles are done anyway

will check it out , thanks, btw how should i now get this changes into my device, a git pull will work right?

Additionally there's a lot of open issues and discussion around the profile and authentication system (#32, #46, #81, #42), some features (eg. changing password) might need to wait until these are implemented, but perhaps this could still be merged into development or a separate branch for managing the new profile and authentication system (to allow you to make sure you still get your hacktoberfest credit in time)

ok man no worries, i have time to complete this work, working on this is more fun tbh

And what about this rufff check fail?

@nfoert
Copy link
Owner

nfoert commented Oct 10, 2024

So something like compression?

Yes, ideally this would help with storage size costs as well, for example a user could upload a really detailed logo that takes up a lot of space, enough users doing this and the cost of storage in production will increase
It doesn't necessarily need to compress the file, but not allowing uploads over 2MB or something could be a good implementation

i have made the alpine js version too, u could access it a profile_a, or did u mean anything, i didnt get it

I think it would be better to move the profile page to just be at /profile instead of having /profile and /profile_a, this will be easier to maintain, and there's some aspects of the two templates that are duplicated, I was under the impression that /profile_a was a more experimental example of alpine.js, unless I'm not understanding your plans correctly?

will check it out , thanks, btw how should i now get this changes into my device, a git pull will work right?

Yes, I've committed the changes to your fork of the code so running git pull will get the new changes from your fork

And what about this rufff check fail?

It seems like it was flagging a double import, which I see you've just committed to fix so it should be all good now!

@nfoert nfoert added the hacktoberfest A good issue for hacktoberfest 2024 label Oct 10, 2024
@spline2hg
Copy link
Author

I think it would be better to move the profile page to just be at /profile instead of having /profile and /profile_a, this will be easier to maintain, and there's some aspects of the two templates that are duplicated, I was under the impression that /profile_a was a more experimental example of alpine.js, unless I'm not understanding your plans correctly?

so yeah i will remove the profile_a and keep only the profile/ that will use alpine js for switching part, does this seem right?

@nfoert
Copy link
Owner

nfoert commented Oct 11, 2024

so yeah i will remove the profile_a and keep only the profile/ that will use alpine js for switching part, does this seem right?

That sounds good to me @spline2hg! Thanks for working on this

@spline2hg
Copy link
Author

@nfoert look into the changes and tell any changes if required,.thanks

@nfoert
Copy link
Owner

nfoert commented Oct 14, 2024

Thanks for the updated changes! I like how it's looking so far
I thought of a few other additions that could be made:

  • Perhaps the profile picture for the user could be shown on the home and index pages on this button? Additionally a button is needed to get to the profile page in the first place, this button could be added here too?
    image
  • It appears that the profile picture is always saved as the name of the uploaded file, this will cause issues if multiple users upload an image called profile.png for example, the files will get overlapped. Perhaps the file name could be changed to a unique ID for each image?

Once this PR is done I'm planning on merging it into a separate branch for all of the profile, authentication, and email changes before all of those features have been implemented. Thanks for your work on this!

@nfoert nfoert changed the base branch from development to feature/new-authentication-profiles-and-emails October 14, 2024 00:41
@spline2hg
Copy link
Author

Additionally a button is needed to get to the profile page in the first place, this button could be added here too?

yes, having a navbar would be great for this regard, this could be a future inprovement, i was thinking of placing the profile button on home page somewhere around the wallet and my cards buttons, and we could either add a similar button on the starting page or make users go to home and then profile, does this work?

image

Would you prefer a single dropdown menu with text options for both profile and logout, without including a profile image? This would be cleaner and more straightforward. Let me know if you'd like to explore other navigation options that might work better for your specific layout and user experience goals.

It appears that the profile picture is always saved as the name of the uploaded file, this will cause issues if multiple users upload an image called profile.png for example, the files will get overlapped. Perhaps the file name could be changed to a unique ID for each image?

i tried this django, its adding a random pattern sequence if the file name already exists, is this fine or should i make changes to give some unique name

Once this PR is done I'm planning on merging it into a separate branch for all of the profile, authentication, and email changes before all of those features have been implemented. Thanks for your work on this!

great

@nfoert
Copy link
Owner

nfoert commented Oct 14, 2024

Would you prefer a single dropdown menu with text options for both profile and logout, without including a profile image?

The best idea I can think of for this would be to modify the existing dropdown to log out and add the button to access the profile and the profile picture there, so we can reuse that element across multiple pages if need be.
Perhaps something like this could work?
image

i tried this django, its adding a random pattern sequence if the file name already exists, is this fine or should i make changes to give some unique name

Ah okay that's good to know, I guess I didn't test it correctly, that's my mistake. Leaving this behavior as it is now is fine with me!

@spline2hg
Copy link
Author

i have added a dropdown at home page for profile page and on profile page to home ,does this seem fine, and? and on the home page the dropdown now disappears if clicked anywhere on the page.

i havent done for the starting page what to do for it, as it currently doesnt have a dropdown, what to d about it?

@nfoert
Copy link
Owner

nfoert commented Oct 15, 2024

i have added a dropdown at home page for profile page and on profile page to home ,does this seem fine, and? and on the home page the dropdown now disappears if clicked anywhere on the page.

This looks great! Thanks for implementing this

i havent done for the starting page what to do for it, as it currently doesnt have a dropdown, what to d about it?

I think it's fine if this page doesn't have a dropdown menu, the button helps give the user a fast way to get to the home page

This feature should be ready to merge! Is there anything else you'd like to implement? Otherwise you can go ahead and convert this PR to no longer be a draft and I'll get it merged for you

@spline2hg
Copy link
Author

I think it's fine if this page doesn't have a dropdown menu, the button helps give the user a fast way to get to the home page

That makes sense

This feature should be ready to merge! Is there anything else you'd like to implement? Otherwise you can go ahead and convert this PR to no longer be a draft and I'll get it merged for you

i was thinking of implementing restricting the image size, for that i was using some validators and raising validation errors when uploading larger than threshold value, but the the image was getting changes nonetheless and it was not showing up on the page.

what i think is i might work on this afterwards in a diff pr , is this fine?apart from this ig most if the things are done , and going forward i will try to do the email authentication,password change and making setting features work.

I will proceed to create a PR from my draft if you don’t have any further suggestions. Thank you so much for all your guidance—I really appreciate it! I look forward to making more contributions to the project.

@nfoert
Copy link
Owner

nfoert commented Oct 16, 2024

i was thinking of implementing restricting the image size, for that i was using some validators and raising validation errors when uploading larger than threshold value

This is a good idea, I think that would be a good thing to implement

what i think is i might work on this afterwards in a diff pr , is this fine

That sounds good to me

i will try to do the email authentication,password change and making setting features work.

Okay sounds good, thanks for working on adding all of these features!

I don't have any more suggestions, this feature is working great! Feel free to convert this PR when you're ready or I can do it as well, you should be able to press a button like Ready for review to convert it to a normal PR

@spline2hg spline2hg marked this pull request as ready for review October 19, 2024 17:07
@spline2hg
Copy link
Author

@nfoert changed the PR to a normal PR. I learned a lot of things working on this project,thank youman. Plz let me know if u have any suggestions for me .

Thank You for the help and collaboration!

@nfoert
Copy link
Owner

nfoert commented Oct 20, 2024

Thanks for finishing up this PR @spline2hg! It looks great to me, I appreciate the work you've done on this, I'm glad we got to collaborate on this together.
I'll get it all merged now!

@nfoert nfoert merged commit 518d2d1 into nfoert:feature/new-authentication-profiles-and-emails Oct 20, 2024
2 checks passed
This was referenced Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest A good issue for hacktoberfest 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants