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

Add support for getting prop, classes and attributes by key #941

Merged
merged 1 commit into from
Aug 26, 2018
Merged

Add support for getting prop, classes and attributes by key #941

merged 1 commit into from
Aug 26, 2018

Conversation

willsoto
Copy link
Contributor

@willsoto willsoto commented Aug 23, 2018

Matches a similar interface from Enzyme:

Just a tiny bit of syntactic sugar to make getting a particular prop easier.

NB: I didn't see an issue for this when I looked, but will be happy to make one if it's necessary.

@eddyerburgh
Copy link
Member

Thanks for this, I've been intending to add it for a while! Could you also update classes, and attributes in this PR to follow this pattern? I'd like to keep the APIs in sync.

@willsoto
Copy link
Contributor Author

Sure! attributes seems pretty obvious. For classes the return value is string[], so do you prefer indexOf or find or something else?

@eddyerburgh
Copy link
Member

Great :)

indexOf, because Array.prototype.find isn't supported by IE or phantom JS

@willsoto
Copy link
Contributor Author

@eddyerburgh and what should the return value for classes be, boolean? Would it behave more like hasClass?

@willsoto
Copy link
Contributor Author

@eddyerburgh I assumed classes('name') should return a boolean. If not, it's easy enough to change 😄

I successfully used function overloading for the .d.ts file, but I am not familiar enough with Flow to know if I did the types there correctly. As a result, I ended up having to alter some functions that utilized attributes() in order to satisfy the type checker. Ideally, I'd just function overload there as well and not have to rewrite any code, so looking for suggestions on that.

Thanks!

@willsoto willsoto changed the title Add support for getting prop by key Add support for getting prop, classes and attributes by key Aug 24, 2018
@@ -13,3 +13,11 @@ import Foo from './Foo.vue'
const wrapper = mount(Foo)
expect(wrapper.attributes().id).toBe('foo')
```

Copy link
Member

Choose a reason for hiding this comment

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

I think we should just move the assertion to the above example, no need to duplicate the set up.

eg:

import { mount } from '@vue/test-utils'
import Foo from './Foo.vue'	 import Foo from './Foo.vue'
 	 
const wrapper = mount(Foo)	const wrapper = mount(Foo)
expect(wrapper.attributes().id).toBe('foo')
expect(wrapper.attributes('id')).toBe('foo')

Copy link
Member

Choose a reason for hiding this comment

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

Same for the rest of the docs pages

@@ -112,9 +115,17 @@ export default class Wrapper implements BaseWrapper {
}
})
classes = classes.map(
className => cssModuleIdentifiers[className] || className
klass => cssModuleIdentifiers[klass] || klass
Copy link
Member

Choose a reason for hiding this comment

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

Why the name change? Can you replace it with c rather than klass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

className ended up shadowing the argument name so I ended up changing it here since it isn't user facing. I am not sure you can use class since it is a reserved word. How about name?

@@ -766,7 +784,8 @@ export default class Wrapper implements BaseWrapper {
*/
setSelected (): void {
const tagName = this.element.tagName
const type = this.attributes().type
const attributes = this.attributes()
const type = typeof attributes === 'object' ? attributes.type : ''
Copy link
Member

Choose a reason for hiding this comment

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

Was this to fix flow errors? If so you can just add // $FlowIgnore above the original line. We know it's never going to return a string, so there's no need to guard against it.

We're actually going to replace flow with TypeScript soon, which would fix this issue. It's better to add flow ignores now rather than add extra code to satisfy flow.

@willsoto
Copy link
Contributor Author

@eddyerburgh Thanks for the feedback. It should all be addressed.

@eddyerburgh
Copy link
Member

Sorry there was another comment that I thought I'd added—can you add an arguments section to the docs, like find—https://vue-test-utils.vuejs.org/api/wrapper/find.html?

… key

Signed-off-by: Will Soto <will.soto9@gmail.com>
@willsoto
Copy link
Contributor Author

@eddyerburgh done and rebased 😄

Copy link
Member

@eddyerburgh eddyerburgh left a comment

Choose a reason for hiding this comment

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

Thanks for this :)

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.

2 participants