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

Issue 3907 profile header update #4605

Merged
merged 13 commits into from
Jul 17, 2019
Merged

Issue 3907 profile header update #4605

merged 13 commits into from
Jul 17, 2019

Conversation

iamonuwa
Copy link
Contributor

@iamonuwa iamonuwa commented Jun 10, 2019

Description

Added a new directory wallpapers inside the assets. It is where the banners will reside so that the api can load them dynamically.

Once a banner is selected, the path to the file is stored against the user's data

Refers/Fixes

Fixes #3907

Testing

Tested locally with the wallpapers on my machine

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

  • squash commits
  • better description
  • screenshot + video (should be attached in PR even if it's added in the issue )

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Jun 12, 2019 via email

@iamonuwa iamonuwa changed the title WIP Issue 3907 profile header update Issue 3907 profile header update Jun 12, 2019
@iamonuwa
Copy link
Contributor Author

@owocki
Copy link
Contributor

owocki commented Jun 12, 2019

thanks! looks like it's functionality working

  1. does the 'update profile' button just show up if it's your own profile? is the endpoint secure such that people can't change each others profile images?
  2. what about this part of the ticket?

Select an image from a list of preselected profile headers. Those profile headers are already created, and they are uploaded to this ticket.

we put a bunch of effort into getting these preselected profile headers together, so it'd be great to see them leveraged.
3. can we position the 'update profile' header on top of the profile header image itself (on the z axis), similar to the twitter example?

ref: #3907

@iamonuwa
Copy link
Contributor Author

@owocki thanks for the quick review.

  1. Just added the security flag to it.
  2. I think of adding a modal with the pre-populated images on either aws or from the project locally + an option to do this. What do you think?
  3. I don't understand this. Can you point it out for me?

@codecov
Copy link

codecov bot commented Jun 12, 2019

Codecov Report

Merging #4605 into master will decrease coverage by 12.62%.
The diff coverage is 30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #4605       +/-   ##
==========================================
- Coverage   30.42%   17.8%   -12.63%     
==========================================
  Files         216     205       -11     
  Lines       17276   17229       -47     
  Branches     2345    2344        -1     
==========================================
- Hits         5256    3067     -2189     
- Misses      11811   14152     +2341     
+ Partials      209      10      -199
Impacted Files Coverage Δ
app/app/urls.py 0% <ø> (-89.37%) ⬇️
app/dashboard/models.py 37.93% <100%> (-17.76%) ⬇️
app/dashboard/views.py 11.45% <25%> (-2.96%) ⬇️
app/dashboard/helpers.py 11.89% <33.33%> (-2.01%) ⬇️
app/kudos/forms.py 0% <0%> (-100%) ⬇️
app/grants/serializers.py 0% <0%> (-100%) ⬇️
app/avatar/serializers.py 0% <0%> (-100%) ⬇️
app/grants/urls.py 0% <0%> (-100%) ⬇️
app/inbox/urls.py 0% <0%> (-100%) ⬇️
app/avatar/urls.py 0% <0%> (-100%) ⬇️
... and 81 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ae1a6c...f0456c9. Read the comment docs.

@owocki
Copy link
Contributor

owocki commented Jun 12, 2019

@iamonuwa

  1. awesome! glad we checked
  2. either one makes sense to me. locally might be simpler, but i defer to you and dan.
  3. checkout what this functionality looks like on twitter. basically we save space on the profile by not having the 'update image' button on the profile, but instead putting it above the cover photo itself.

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Jun 12, 2019

Awesome, for the update button profile I was trying to replicate the twitter's Edit Profile button. Once you click on it, it should give you the Change your header photo. Anywhere around the background will open up the modal in the case of 1.

@owocki

@owocki
Copy link
Contributor

owocki commented Jun 12, 2019

thanks for the update. i probably sould have specified better in the ticket. i think having the 'update your profile header' onload (IFF the user is looking at their own profile) is a better ux

@iamonuwa
Copy link
Contributor Author

thanks for the update. i probably sould have specified better in the ticket. i think having the 'update your profile header' onload (IFF the user is looking at their own profile) is a better ux

Understood, ATM the currently logged in user alone sees the button

@iamonuwa
Copy link
Contributor Author

@thelostone-mc @owocki @PixelantDesign @danlipert new update. I've taken care of the changes. Please take a look. https://www.dropbox.com/s/pt7ia6jqixdlana/Header%20Image.mp4?dl=0

@iamonuwa
Copy link
Contributor Author

Currently having issue pushing updates to GitHub. Will keep trying

@owocki
Copy link
Contributor

owocki commented Jun 13, 2019

looking good!

  1. can we remove the 'update profile' button and instead just always have the ability to update our own header images?
  2. if i add an image, does that make it avaialble only to me, or to everyone on the platfrom? im a little worried about ppl uploading inappropriate images.

@iamonuwa
Copy link
Contributor Author

looking good!

  1. can we remove the 'update profile' button and instead just always have the ability to update our own header images?
  2. if i add an image, does that make it avaialble only to me, or to everyone on the platfrom? im a little worried about ppl uploading inappropriate images.
  1. Personally, I think the current implementation is a better UX except if I don't get what you mean. Not cool to show the Change your header photo once I open my profile.
  2. Its public. We can force everyone to use the platform specific banners if that will solve the inappropriate images issue.

@owocki
Copy link
Contributor

owocki commented Jun 13, 2019

  1. ok that works
  2. i like that suggestion

@iamonuwa
Copy link
Contributor Author

  1. i like that suggestion

Am going to remove user file upload, everyone has to use Gitcoin's preselected banners

@iamonuwa
Copy link
Contributor Author

@owocki
Copy link
Contributor

owocki commented Jun 13, 2019 via email

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Jun 13, 2019

can we preload the banners we designed into the modal?

Just load the files into the wallpapers directory. Its located inside the assets directory. Relax and watch the magic happen. 😂🔥🔥🔥🔥🔥

@iamonuwa
Copy link
Contributor Author

@thelostone-mc @owocki @octavioamu ready for review

@owocki
Copy link
Contributor

owocki commented Jun 14, 2019

Just load the files into the wallpapers directory. Its located inside the assets directory. Relax and watch the magic happen. 😂🔥🔥🔥🔥🔥

shouldnt that be part of this PR?

@iamonuwa
Copy link
Contributor Author

Just load the files into the wallpapers directory. Its located inside the assets directory. Relax and watch the magic happen. joyfirefirefirefirefire

shouldnt that be part of this PR?

I have a sample inside the directory. I can add the available ones from the creatives repo. Moving forward, that's where the wallpapers will live

@owocki
Copy link
Contributor

owocki commented Jun 14, 2019 via email

@iamonuwa
Copy link
Contributor Author

@octavioamu image

@iamonuwa
Copy link
Contributor Author

@owocki do you want to support gifs? That will need extra engineering. What do you think?

@iamonuwa
Copy link
Contributor Author

app/assets/v2/js/pages/profile.js Outdated Show resolved Hide resolved
app/dashboard/views.py Outdated Show resolved Hide resolved
@owocki
Copy link
Contributor

owocki commented Jul 10, 2019

very excited for this to finally go live!

danlipert
danlipert previously approved these changes Jul 10, 2019
thelostone-mc
thelostone-mc previously approved these changes Jul 17, 2019
@danlipert
Copy link
Contributor

@octavioamu please review

@octavioamu octavioamu dismissed stale reviews from thelostone-mc and danlipert via 2b5845e July 17, 2019 14:16
octavioamu
octavioamu previously approved these changes Jul 17, 2019
Copy link
Contributor

@octavioamu octavioamu left a comment

Choose a reason for hiding this comment

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

lgtm

@danlipert danlipert merged commit 6077f2d into gitcoinco:master Jul 17, 2019
@owocki
Copy link
Contributor

owocki commented Jul 17, 2019

i get this expection when trying to run on my local:

Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/django/contrib/staticfiles/handlers.py", line 65, in __call__
    return self.application(environ, start_response)
  File "/usr/local/lib/python3.7/site-packages/django/core/handlers/wsgi.py", line 142, in __call__
    response = self.get_response(request)
  File "/usr/local/lib/python3.7/site-packages/django/core/handlers/base.py", line 78, in get_response
    response = self._middleware_chain(request)
  File "/usr/local/lib/python3.7/site-packages/django/core/handlers/exception.py", line 36, in inner
    response = response_for_exception(request, exc)
  File "/usr/local/lib/python3.7/site-packages/django/core/handlers/exception.py", line 90, in response_for_exception
    response = handle_uncaught_exception(request, get_resolver(get_urlconf()), sys.exc_info())
  File "/usr/local/lib/python3.7/site-packages/django/core/handlers/exception.py", line 125, in handle_uncaught_exception
    return debug.technical_500_response(request, *exc_info)
  File "/usr/local/lib/python3.7/site-packages/django_extensions/management/technical_response.py", line 37, in null_technical_500_response
    six.reraise(exc_type, exc_value, tb)
  File "/usr/local/lib/python3.7/site-packages/six.py", line 692, in reraise
    raise value.with_traceback(tb)
  File "/usr/local/lib/python3.7/site-packages/django/core/handlers/exception.py", line 34, in inner
    response = get_response(request)
  File "/usr/local/lib/python3.7/site-packages/django/core/handlers/base.py", line 126, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/usr/local/lib/python3.7/site-packages/django/core/handlers/base.py", line 124, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/code/app/dashboard/views.py", line 1746, in load_banners
    images = load_files_in_directory('wallpapers')
  File "/code/app/dashboard/helpers.py", line 59, in load_files_in_directory
    for f in os.listdir(path):
FileNotFoundError: [Errno 2] No such file or directory: '/code/app/static/wallpapers'

@owocki
Copy link
Contributor

owocki commented Jul 17, 2019

note that i have not run collectstatic, and neither will the production servers have run it (there are multiple boxes and only ONE will run collectstatic on it)

@danlipert
Copy link
Contributor

Just for posterity, when this got rolled to production I created the folder manually and touched the filenames on each server

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants