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

Show logo on screens between 768px and 998px - #838 #982

Merged

Conversation

robert9g
Copy link
Contributor

@robert9g robert9g commented Nov 2, 2017

Fixes, one part of #838

Changes:

  • moved .center's margin-left from @medium to @small, so that the logo will not be covered by the search field

@bonustrack bonustrack temporarily deployed to busy-nd-pr-982 November 3, 2017 10:57 Inactive
@bonustrack
Copy link
Contributor

There should be a right border on the search input and it should end at the level of the red line:
image

@bonustrack bonustrack temporarily deployed to busy-nd-pr-982 November 6, 2017 12:03 Inactive
@robert9g
Copy link
Contributor Author

robert9g commented Nov 6, 2017

You're right, I will do that now.

@bonustrack bonustrack temporarily deployed to busy-nd-pr-982 November 6, 2017 13:10 Inactive
@mktcode
Copy link
Contributor

mktcode commented Nov 6, 2017

And there's a padding missing.

screenshot-2017-11-6 busy

@robert9g
Copy link
Contributor Author

robert9g commented Nov 6, 2017

@mktcode I noticed that before, but left it alone because it was not directly related to the current issue, namely that the logo was hidden. And I didn't want to make unrelated changes. But it can be easily fixed yes.

@bonustrack bonustrack temporarily deployed to busy-nd-pr-982 November 6, 2017 14:23 Inactive
@mktcode
Copy link
Contributor

mktcode commented Nov 6, 2017

@sirrius-a-b I just realized that it's the same on the other side.

screenshot-2017-11-6 busy 1

Maybe you wanna fix that too. I would consider this "related". I don't think there's need for a separate PR. :)

@robert9g
Copy link
Contributor Author

robert9g commented Nov 6, 2017

Let's wait and see what the members have to say about this.

@mktcode
Copy link
Contributor

mktcode commented Nov 6, 2017

@sirrius-a-b updated my privacy settings... :D
Actually I don't really know why I am a member but... pssst! Yet I feel confident enough to say that a "fixed right padding" commit would be totally reasonable for this PR. ;)

@bonustrack bonustrack temporarily deployed to busy-nd-pr-982 November 7, 2017 13:58 Inactive
@robert9g
Copy link
Contributor Author

robert9g commented Nov 7, 2017

The logo is showing up, and everything is lined-up :) What do you think?

@mktcode
Copy link
Contributor

mktcode commented Nov 8, 2017

@bonustrack I think this can be merged now.... but @sirrius-a-b you have to update your branch first.

@bonustrack bonustrack temporarily deployed to busy-nd-pr-982 November 8, 2017 11:50 Inactive
@bonustrack bonustrack temporarily deployed to busy-nd-pr-982 November 8, 2017 21:33 Inactive
@robert9g
Copy link
Contributor Author

I updated the branch, on several occasions. Please let me know if there are other issues with this PR.

@robert9g
Copy link
Contributor Author

@bonustrack I am not sure why this is not worthy of being merged. Please review it if you have some time. Or I could close it if it's unwanted for some reason.

Copy link
Contributor

@bonustrack bonustrack left a comment

Choose a reason for hiding this comment

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

Looking good thank you!

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