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

Remove padding top from navbar site logo #1935

Merged
merged 4 commits into from
May 23, 2022

Conversation

elroygohjy
Copy link
Contributor

@elroygohjy elroygohjy commented May 23, 2022

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Overview of changes:
Resolves #1895

Anything you'd like to highlight / discuss:
Remove padding top from .nav-left class selector.

Before:
image
After:
image

Testing instructions:
Inspect navbar site logo and it is now align-center with other navbar elements.

Proposed commit message: (wrap lines at 72 characters)
Remove padding top from site logo

@damithc
Copy link
Contributor

damithc commented May 23, 2022

Thanks for the PR @elroygohjy Indeed the 'after' one looks better.
🤔 hmmm... are we sure we want to change a general navbar style like this? It will affect every MarkBind site. Could the problem lie with the specific logo image rather than the style?

@elroygohjy
Copy link
Contributor Author

elroygohjy commented May 23, 2022

Hello prof,
This problem is definitely not caused by the logo used.
So, current logo height is 20px as shown:
image
But, there is addition 10px of padding-top that is hidden, although the height of navbar-brand should be 30px inclusive of padding, and it is not aligning center.
image
This is the reason why the logo is not aligning.
My current implementation failed to understand this problem, i will be creating a new commit that removes this 10px of padding and to center the logo.

Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Thanks for investigating and fixing this @elroygohjy. Nice work :)

Just one question, the rest looks good :)

@@ -363,12 +363,18 @@ export default {
display: inline-block;
}

.navbar-brand > img,
svg {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
svg {
.navbar-brand > svg {

Should this have the .navbar-brand selector as well? If not this will select all svg elements.

Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jonahtanjz jonahtanjz added this to the 4.0 milestone May 23, 2022
@jonahtanjz jonahtanjz merged commit 05faed3 into MarkBind:master May 23, 2022
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.

Off-center NavBar items
3 participants