Skip to content

Wrapper: find/findAll accepts ref options object #68

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

Merged
merged 1 commit into from
Nov 10, 2017

Conversation

matt-oconnell
Copy link
Contributor

This PR relates to #67

It adds the .ref('someRefName') method to the main wrapper.

This returns a WrapperArray with wrappers for any vnodes that have the specified ref. Additionally, .ref will only match vnodes with refs that exist on the wrapper.vm.$refs.

  1. Do we want to just have a single ref function or do we want to split it into findRefs / findAllRefs? Right now, this only returns a wrapper array.

  2. I haven't implemented tests yet because I wanted to make sure I was going in the right direction first.

  3. The find-matching-vnodes-by-ref.js file that is added here uses some of the functionality from the existing find-matching-vnodes.js file. Should I pull out some of those functions into a more generic vnode traversal file or something?

@eddyerburgh
Copy link
Member

  1. I'm not sure on the API. Ideally we would be able to extend find and findAll to handle refs. Let's discuss in Find / FindRef: wrapper function to find element based on ref #67
  2. That's fine, but we'll need some before me merge
  3. Yes 🙂

export default function findVNodesByRef (vNode: VNode, refName: string): Array<VNode> {
const nodes = findAllVNodes(vNode)
const refFilteredNodes = nodes.filter(node => nodeMatchesRef(node, refName))
// Only return ref matches from top level vNode

Choose a reason for hiding this comment

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

For my own understanding, why do we not do a recursive check here? Might be helpful to document in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. This is because we want to emulate the exact selector functionality of wrappedComponent.$refs.myRef.

If we do a recursive check, we may return a general result of wrappedComponent.$refs.someComponent.$refs.someComponent.$refs.myRef when we only want to grab ref elements on the top-level wrapper itself. If you need a sub-component ref, you'd need to find the subcomponent and get the ref off of it explicitly.

Choose a reason for hiding this comment

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

Cool, thanks for the 💯 explanation!

@matt-oconnell matt-oconnell changed the title wrapper.refs Wrapper: find/findAll accepts ref options object Oct 3, 2017
const nodes = findAllVNodes(vNode)
const filteredNodes = nodes.filter(node => nodeMatchesSelector(node, selector))
return removeDuplicateNodes(filteredNodes)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file includes all non-shared functionality from find-matching-vnodes

}
})
return uniqueNodes
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file includes all shared functionality from find-vnodes-by-selector and find-vnodes-by-ref

@matt-oconnell matt-oconnell force-pushed the find-refs branch 8 times, most recently from f7454b3 to 4e901ba Compare October 4, 2017 15:01
@matt-oconnell
Copy link
Contributor Author

Ok, @eddyerburgh, so I know we're waiting on the discussion to resolve in #67 about whether or not the find by $ref functionality should be implemented for all methods that take a "Selector," but this PR implements that functionality in the case that we do decide to move forward with that.

A few notes:

  1. Importing flow-types from vue does not seem to work correctly. Selector was defined as
declare type Selector = string | Component

but it looks like the global imports in flow/vue.js may not actually get imported. So, anywhere that accepted a Selector type was actually accepting any. What should be done about this?

  1. is throws an error if an options object { ref: 'someName' } is passed in because we can't assert that an element matches the $ref selector without access to the component that has that $ref property. (it would be like asking: "is this component a ref of this component")

  2. We throw errors when when trying to select by $ref on non-Vue components.

  3. The naming of this new selector type is currently RefSelector. There may a better way to refer to it

  4. Will need to update docs

@eddyerburgh
Copy link
Member

As you said, we should hold off until we've had more input from the community. But it looks great so far 🙂

  1. I'll look into this
  2. OK that makes sense
  3. Good
  4. I think that's fine
  5. OK, are you able to do this?

@matt-oconnell
Copy link
Contributor Author

re: 5. Yep, this is in the PR now.

@kaicataldo
Copy link

@eddyerburgh Do you have a link to the discussion that is happening around this? Would love to see this land!

@eddyerburgh
Copy link
Member

@kaicataldo here — #67

@eddyerburgh
Copy link
Member

Hey @matt-oconnell , are you able to fix the merge conflicts? Then I'll merge

@matt-oconnell matt-oconnell force-pushed the find-refs branch 3 times, most recently from 883999a to 754cee1 Compare November 10, 2017 21:15
@matt-oconnell
Copy link
Contributor Author

Ok, resolved 👍

@eddyerburgh eddyerburgh merged commit 24bfb95 into vuejs:dev Nov 10, 2017
@eddyerburgh
Copy link
Member

Thanks @matt-oconnell 😀

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