-
Notifications
You must be signed in to change notification settings - Fork 964
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
Updated profile pages to show username if not viewing own profile #3837
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @coreygirard! Looks like there's a CSP (Content-Security-Policy) violation that needs fixed up.
</div> | ||
{% else %} | ||
<div class="sidebar-section"> | ||
<div style="height:20px"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lint violation is due to the fact that our Content Security Policies do not allow inline style. You'll need to update the SCSS in https://github.com/pypa/warehouse/blob/053c890c8b305cb5ca03de718866ba600915ab40/warehouse/static/sass/blocks/_sidebar-section.scss#L27 or find an appropriate class to apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note that the linter can be run locally with make lint
Thanks for the quick update! Corey did this at an NYC Python sprint night tonight - glad to meet you! |
My pleasure! And likewise! |
@@ -14,4 +14,16 @@ | |||
|
|||
{% if request.user == user %} | |||
<a href="{{ request.route_path('manage.account') }}" class="button button--primary author-profile__edit-button">Edit profile</a> | |||
|
|||
<div class="sidebar-section"> | |||
<div class="sidebar-section__spacer"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @coreygirard, thanks for this PR !
I think we can avoid adding an empty div here, instead we can add a modifier to sidebar-section
.
e.g. sidebar-section sidebar-section--spaced
.
Then we can add a margin-top: 20px
style in the css.
Typically, we add modifiers at the bottom of the codeblock like this &--spaced
, and add a small comment in the documentation at the top of the page. In this case, there seems to also be some out of date comments there too.
The button group CSS file is a good place to look for a good example :)
Let me know if this isn't clear, or if you'd like me to make these changes for you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I had originally tried to do basically exactly that, but for some reason, it didn't end up showing the space. I just tried again, with no luck. But my CSS skills are quite limited, so if you wouldn't mind making the edit, that would be much appreciated :)
Hi @coreygirard , I've made an updated PR here that includes your commits, so I'm going to close this in favour of that PR :) |
Closes #3591
When signed in and viewing own profile
When signed out or viewing someone else's profile