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

Update outbound link icon alignment #1308

Merged
merged 1 commit into from
Feb 24, 2019

Conversation

gtsiolis
Copy link
Contributor

@gtsiolis gtsiolis commented Feb 17, 2019

Summary

This will update outbound link icon alignment.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Before (header) After (header)
image image
Before (paragraph) After (paragraph)
image image

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

@gtsiolis gtsiolis force-pushed the gt-update-outbound-link-icon-alignment branch from f1c74e0 to 506b788 Compare February 17, 2019 20:54
@gtsiolis
Copy link
Contributor Author

@ulivz could you review or forward this?

Copy link
Member

@ulivz ulivz left a comment

Choose a reason for hiding this comment

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

You would need some hacks to achieve horizontal centering of inline-block and inline elements in the line, you would need all other inline elements to be set vertical-align which is not very elegant in VuePress, and your fix here is not really centered.

This is my fix:

  • Code change:
-    vertical-align: text-bottom;
+   vertical-align: middle;
+   position: relative;
+   top: -1px;
}
  • Snapshot

image

It's also centered when you change the font-size:

image

Isn't better?

@gtsiolis
Copy link
Contributor Author

Thanks @ulivz! The goal was not to vertically align the icon but actually use text-bottom alignment in the first place. I think that both approaches are acceptable but let me know if you think this should vertically align the icon in the middle instead.

@shigma
Copy link
Collaborator

shigma commented Feb 18, 2019

It's also centered when you change the font-size:

I think outbound link will look better if keeping the same size as fonts.

@ulivz
Copy link
Member

ulivz commented Feb 18, 2019

@shigma Yup but we can do it in another PR, since there are two things.

@shigma
Copy link
Collaborator

shigma commented Feb 18, 2019

@shigma Yup but we can do it in another PR, since there are two things.

I bet you are right.

@gtsiolis gtsiolis force-pushed the gt-update-outbound-link-icon-alignment branch from 506b788 to 4d1ac0f Compare February 24, 2019 15:38
@gtsiolis
Copy link
Contributor Author

gtsiolis commented Feb 24, 2019

@ulivz I tried this out with a couple of different font sizes and the suggestion in #1308 (review) looks much better for larger fonts within Vuepress. I've updated the code and the screenshots in the description. Let me know what you think.

Copy link
Member

@ulivz ulivz left a comment

Choose a reason for hiding this comment

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

Cannot wait to merge it!

@ulivz ulivz merged commit 6de1c30 into vuejs:master Feb 24, 2019
@gtsiolis
Copy link
Contributor Author

Thanks @ulivz! 😆

@gtsiolis gtsiolis deleted the gt-update-outbound-link-icon-alignment branch February 24, 2019 16:07
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