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

Revert menu icon to white #1445

Merged
merged 3 commits into from Sep 9, 2017
Merged

Revert menu icon to white #1445

merged 3 commits into from Sep 9, 2017

Conversation

ghost
Copy link

@ghost ghost commented Sep 7, 2017

No description provided.

@strugee
Copy link
Member

strugee commented Sep 7, 2017

I can't figure out what this actually changes? I ran document.querySelector('.navbar .btn-navbar span') and the result was null so this doesn't seem to match anything?

@ghost
Copy link
Author

ghost commented Sep 7, 2017

You are logout?

pump login icon

@strugee
Copy link
Member

strugee commented Sep 7, 2017

Ahhh, thanks. It only appears on small viewports too.

Reviewing.

Copy link
Member

@strugee strugee left a comment

Choose a reason for hiding this comment

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

Got some nits inline but overall LGTM 👍

Thanks for catching this!

@@ -35,6 +35,10 @@ body {
width: auto;
}

.navbar .btn-navbar span {
color: white;
Copy link
Member

Choose a reason for hiding this comment

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

Two spaces, not four, for consistency with surrounding lines

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I have emacs configuration by default in four XD, I will change

@@ -35,6 +35,10 @@ body {
width: auto;
}

.navbar .btn-navbar span {
Copy link
Member

Choose a reason for hiding this comment

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

This would be more clear if it targeted .fa.fa-bars

Copy link
Author

@ghost ghost Sep 7, 2017

Choose a reason for hiding this comment

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

I think specifics styles are harder to maintain and adds unnecessary complexity, the result is the same.

If in the future changes the icon name no need change styles only replace the class or if changes the icon family only need to change classes

https://stackoverflow.com/questions/2281766/generic-vs-specific-element-styles-for-maintainability

Copy link
Member

Choose a reason for hiding this comment

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

I'd be okay with that if you add a comment above saying what this applies to. Otherwise future readers may have the same question I did ("what does this even apply to?")

Copy link
Author

Choose a reason for hiding this comment

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

Made

Copy link
Member

@strugee strugee left a comment

Choose a reason for hiding this comment

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

Thanks!

@strugee strugee merged commit a6a5761 into pump-io:master Sep 9, 2017
@ghost ghost deleted the fix-menu-icon branch October 15, 2017 01:26
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.

1 participant