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

Adds icons to navigation #499

Merged
merged 11 commits into from
Dec 23, 2014
Merged

Adds icons to navigation #499

merged 11 commits into from
Dec 23, 2014

Conversation

magsout
Copy link
Member

@magsout magsout commented Dec 19, 2014

I have updated the icons:

  • Added .wc-Icon (naming conventions)
  • Added icon to navigation (home - all issues - log out)

r? @calexity

@magsout
Copy link
Member Author

magsout commented Dec 19, 2014

Hum travis failed, let's try to git rebase and git push --force

@magsout
Copy link
Member Author

magsout commented Dec 19, 2014

@miketaylr maybe it's because I added another link on navigation ?

@miketaylr
Copy link
Member

@magsout yeah, probably something similar. The test failures are all related to not logging in or logging out:

.findByCssSelector('.wc-Navbar-section--right .wc-Navbar-link').click()

So it wouldn't be hard to break that. Maybe if we add a .login-link class and update the tests they won't break as often. ^_^

@calexity
Copy link
Contributor

Hooray for this! Can you make login say Log in ?

On Fri, Dec 19, 2014, 7:28 AM Mike Taylor [email protected] wrote:

@magsout https://github.com/magsout yeah, probably something similar.
The test failures are all related to not logging in or logging out:

.findByCssSelector('.wc-Navbar-section--right .wc-Navbar-link').click()

So it wouldn't be hard to break that. Maybe if we add a .login-link class
and update the tests they won't break as often. ^_^


Reply to this email directly or view it on GitHub
#499 (comment)
.

@magsout
Copy link
Member Author

magsout commented Dec 19, 2014

@calexity ok .

Hum question, with new link (all issues) there is enought space for small device

capture d ecran 2014-12-19 a 20 25 30

I wonder if display only the icon is enough?

@miketaylr
Copy link
Member

We could give it a shot and see if people find it confusing.

@magsout
Copy link
Member Author

magsout commented Dec 20, 2014

@miketaylr @calexity

Maybe we need an icon for log in ?

webcompat_navigation

@calexity
Copy link
Contributor

@magsout You are right. I would use the same icon for Log in and Log out.

@miketaylr
Copy link
Member

Yeah @magsout, would better if both login/logout had an icon.

@miketaylr
Copy link
Member

Tests are passing, so let's pull this in.

miketaylr pushed a commit that referenced this pull request Dec 23, 2014
Fixes #450 - Adds icons to navigation
@miketaylr miketaylr merged commit 2b22754 into master Dec 23, 2014
@miketaylr miketaylr deleted the icon-header branch December 23, 2014 16:20
@magsout
Copy link
Member Author

magsout commented Dec 23, 2014

Nice, thanks for your help @miketaylr

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