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

feat(autocomplete-matches): use virtualNode only lookups #1604

Merged
merged 3 commits into from Jun 5, 2019

Conversation

straker
Copy link
Contributor

@straker straker commented May 31, 2019

Autocomplete-matches uses the JavaScript DOM property .type to look at the type of input. This is trivial when you have a DOM node, but becomes a bit more complicated when you don't (e.g. AST implementations). Created a method on the VirtualNode that just returns the DOM property lookup but is expected to be overridden for other implementations.

It also tries to determine if the node is visible in the DOM, but that doesn't work for virtual node only runs, so I made it conditional on there being an actualNode to look at.

Please double check the tests that I added all the same properties when converting to queryFixture.

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: @JKODU

@straker straker requested a review from a team May 31, 2019 18:01
Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

LGTM but please wait for a review from someone with more "rules" experience.

lib/core/base/virtual-node.js Outdated Show resolved Hide resolved
jeeyyy
jeeyyy previously approved these changes Jun 3, 2019
lib/core/base/virtual-node.js Outdated Show resolved Hide resolved
@straker straker dismissed stale reviews from jeeyyy and stephenmathieson via a4538af June 4, 2019 20:16
@straker straker requested a review from a team as a code owner June 4, 2019 20:16
Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

LGTM. Please get an OK from @WilcoFiers first tho 😉

@straker straker merged commit b32d4fe into develop Jun 5, 2019
@straker straker deleted the autocompleteMatches branch June 5, 2019 15:13
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

4 participants