Skip to content
This repository was archived by the owner on Nov 25, 2021. It is now read-only.

Conversation

@ijsnow
Copy link
Contributor

@ijsnow ijsnow commented Aug 16, 2018

This PR fixes two closely related issues

  1. @chrismwendt asked if it were possible for the commonAncestorContainer to be too high up in the tree and I, incorrectly, assumed that it couldn't. This PR fixes that.
  2. The previous PR fixed the problem where we highlight only part of a token by refactoring getTokenAtPosition. We still had a problem with placing the tooltip because the event targets weren't guaranteed to be the full token so we use getTokenAtPosition to correct that.

@ijsnow ijsnow requested a review from chrismwendt August 16, 2018 18:53
@codecov
Copy link

codecov bot commented Aug 16, 2018

Codecov Report

Merging #43 into master will decrease coverage by 0.74%.
The diff coverage is 41.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   66.82%   66.08%   -0.75%     
==========================================
  Files          15       15              
  Lines         615      631      +16     
  Branches      161      165       +4     
==========================================
+ Hits          411      417       +6     
- Misses        204      214      +10
Impacted Files Coverage Δ
src/hoverifier.ts 58.56% <0%> (-3.43%) ⬇️
src/token_position.ts 91.27% <100%> (+0.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9938078...4b15907. Read the comment docs.

@ijsnow ijsnow merged commit 2ed1850 into master Aug 16, 2018
@ijsnow ijsnow deleted the get-full-token-fix branch August 16, 2018 18:58
@sourcegraph-bot
Copy link

🎉 This PR is included in version 3.7.2 🎉

The release is available on:

Your semantic-release bot 📦🚀


// Otherwise, we can't guarantee that the common ancester container doesn't contain
// whitespace or other characters around it. To solve for this case, we'll just
// surround the contents of the range with a new span.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the range span multiple lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't think of a case where a token would span multiple lines. Can you?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants