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 Dropdown to user to Header #738

Merged
merged 12 commits into from
Oct 2, 2015
Merged

Added Dropdown to user to Header #738

merged 12 commits into from
Oct 2, 2015

Conversation

magsout
Copy link
Member

@magsout magsout commented Oct 1, 2015

Need to populate link inside dropdown
When button is clicked, just added as usual is-active to wc-DropdownHeader
and for accessibility change value of aria-pressed

r? @miketaylr

@magsout
Copy link
Member Author

magsout commented Oct 1, 2015

Ah hum and yes, I need to rewrite some test about logout user.

@miketaylr
Copy link
Member

🎈

@magsout
Copy link
Member Author

magsout commented Oct 1, 2015

Just pushed my last commit for these beautiful and awesome days.
Thanks again @miketaylr @karlcow @hallvors . Love u guys .

@karlcow
Copy link
Member

karlcow commented Oct 2, 2015

aaaaw ❤️ Thanks a lot for the amazing work.

@miketaylr
Copy link
Member

🎉

@miketaylr
Copy link
Member

Ah hum and yes, I need to rewrite some test about logout user.

I'm happy to re-write the logout test stuff, no worries.

@miketaylr
Copy link
Member

Ah, actually I changed the test yesterday that instead of clicking on a link, it just navigates directly to the /logout route -- so no test bustage here.

{% if session.user_id and session.avatar_url %}
<img class="wc-Navbar-avatar" src="{{ session.avatar_url }}s=40" alt="User Avatar">
{% endif %}

This comment was marked as abuse.

@miketaylr
Copy link
Member

And I guess we can make this change for nav.html as well.

@magsout
Copy link
Member Author

magsout commented Oct 2, 2015

ah yes, it's a different view. I forgot it. There is a way to mixe this two views ?

@miketaylr
Copy link
Member

I was just wondering about that. Looking at them both, we can pull out the "All issues" "login" "logout" links into its own partial and at least share that code.

@miketaylr
Copy link
Member

I just pulled out the common stuff and made a shared-nav-links.html partial, so we're sharing those in top.html and nav.html. Ideally we could just have the same file though, let me see if there's a way in Flask to know what page we're on and we can do some kind of if/else display block (or something).

@miketaylr
Copy link
Member

@magsout I rebased against master (and force pushed) just now because I need to fix the login test, so please do a git pull --rebase or similar before trying to push on this branch. ^_^

edit: I guess I didn't need to, but wanted to. 😎

@miketaylr miketaylr mentioned this pull request Oct 2, 2015
@miketaylr
Copy link
Member

@magsout just a thought, we could land this on master now if we commented out the "My Activity" link. Then we could work on the me-dashboard in a new branch. WDYT?

@magsout
Copy link
Member Author

magsout commented Oct 2, 2015

yeah, looks good to me.

@miketaylr
Copy link
Member

Cool, I'll comment that out and then merge this.

@miketaylr
Copy link
Member

r+ for @magsout work. I'll ask for review on the JS additions when we land the dashboard (or something).

miketaylr pushed a commit that referenced this pull request Oct 2, 2015
Added Dropdown to user to Header
@miketaylr miketaylr merged commit 3381b96 into master Oct 2, 2015
@miketaylr miketaylr deleted the dropdown-user branch October 2, 2015 14:35
@magsout
Copy link
Member Author

magsout commented Oct 2, 2015

awesome

@miketaylr
Copy link
Member

ahahahah

@magsout
Copy link
Member Author

magsout commented Oct 2, 2015

Call me : Mr GIF

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.

3 participants