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

Interaction helpers also accept HTMLElements as first argument #28

Merged
merged 1 commit into from Mar 2, 2017

Conversation

cibernox
Copy link
Owner

@cibernox cibernox commented Mar 2, 2017

Closes #27

@cibernox cibernox requested a review from rwjblue March 2, 2017 12:23
@cibernox cibernox force-pushed the allow-interaction-helper-to-receive-dom-elements branch from e3745b9 to f08a415 Compare March 2, 2017 12:25
@rwjblue
Copy link
Collaborator

rwjblue commented Mar 2, 2017

Hmm. Since all of the other helpers go through find, findAll, and findWithAssert to convert a selector to an element, I was thinking that those would be the only methods that needed modification...

@rwjblue
Copy link
Collaborator

rwjblue commented Mar 2, 2017

Just thinking that would remove a bunch of extra conditionals (and reduce overall complexity for individual helpers), not sure if I've missed something though.

@cibernox
Copy link
Owner Author

cibernox commented Mar 2, 2017

I find semantically weird to call find with the element I already want it to find.

find(button); // button

Feels weird.

@rwjblue
Copy link
Collaborator

rwjblue commented Mar 2, 2017

Ya, I agree that it's odd for users to do this, however it is quite a bit nicer to funnel the concept of "convert selector or element to an element" through a single path (versus forcing every helper to have these kinds of conditionals).

One way we could remove the user calling find(button) case is by moving the core logic of find into a private helper called _ensureElement. Then you could use that helper throughout the other helpers, and still have an assertion if end users call find without a string.

@cibernox
Copy link
Owner Author

cibernox commented Mar 2, 2017

I think that I'd rather do that, since allowing find(HTMLElement) in the public API is rather useless and not something real users would do. Changing it.

@cibernox cibernox force-pushed the allow-interaction-helper-to-receive-dom-elements branch from f08a415 to 6dead7f Compare March 2, 2017 13:44
@cibernox
Copy link
Owner Author

cibernox commented Mar 2, 2017

Done. There is two private helpers. Technically find(HTMLElement) will still return the HTMLElement, but it's not documented in the public API. When we transform this to Typescript it will be enforced, not it's just part of the JSdocs.

@cibernox cibernox merged commit 48436d0 into master Mar 2, 2017
@cibernox cibernox deleted the allow-interaction-helper-to-receive-dom-elements branch March 2, 2017 14:05
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