Navigation Menu

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

Support link underlines in the DOM renderer #1705

Merged
merged 4 commits into from Sep 21, 2018
Merged

Support link underlines in the DOM renderer #1705

merged 4 commits into from Sep 21, 2018

Conversation

agurod42
Copy link

Closes #1704

@Tyriar
Copy link
Member

Tyriar commented Sep 21, 2018

I believe we should consider the border-bottom approach in this case. But of course, you've the last word 😊

I don't actually see that AA issue in Chrome 69, is that the browser you're using? I would prefer to use text-decoration as it simplified the logic a lot plus gives the nice perk of fancy underlines:

screen shot 2018-09-21 at 7 15 38 am

I removed box-sizing: border-box, are you still able to reproduce the AA problem on your mac?

@Tyriar Tyriar added this to the 3.8.0 milestone Sep 21, 2018
@Tyriar Tyriar self-assigned this Sep 21, 2018
@agurod42
Copy link
Author

I was testing on vscode itself. But it also happens on my Chrome:

screen shot 2018-09-21 at 11 56 36 am

I noticed also, that if you zoom the image you posted just before the gaps are also there:

screen shot 2018-09-21 at 11 56 23 am

@agurod42
Copy link
Author

agurod42 commented Sep 21, 2018

Interesting fact: The AA problem disappears if the span width is a number without decimals (By default it computes to 7.234375px in my environment when using font-size 12).

screen shot 2018-09-21 at 12 31 33 pm

@Tyriar
Copy link
Member

Tyriar commented Sep 21, 2018

Lol, it was there on mine, I didn't even notice. I suspect this issue isn't present on lower DPI screens and I say we go ahead with this as is as it's barely noticeable. Thoughts @agurodriguez?

Also, the character width has to be a decimal for the DOM renderer, this uses the width the font specifies.

@agurod42
Copy link
Author

Yeahp, I do agree it's barely noticeable. Let's go ahead.

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.

None yet

2 participants