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: add alternative tag for element selection #1012

Merged
merged 2 commits into from
Dec 14, 2022
Merged

feat: add alternative tag for element selection #1012

merged 2 commits into from
Dec 14, 2022

Conversation

pataar
Copy link
Contributor

@pataar pataar commented Dec 14, 2022

This adds the alternative data-dusk="tag" attribute. As described in: https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes: attributes that are not part of the spec should be prefixed with data-.

Currently TSX does not allow any attributes that aren't defined in the spec, unless they have a data prefix. So without an alternative attribute to dusk=, TSX users won't be able to use Dusk as documented.

@pataar pataar marked this pull request as ready for review December 14, 2022 18:15
@taylorotwell taylorotwell merged commit eedf08c into laravel:7.x Dec 14, 2022
@pataar pataar deleted the feature/add_alternative_dusk_tag branch December 14, 2022 18:46
@adrienne
Copy link

hooray!

@driesvints
Copy link
Member

We just reverted this because it was breaking Nova's test suite.

@crynobone
Copy link
Member

crynobone commented Dec 16, 2022

Hi,

I do believe this is a great addition to Dusk and Nova should use data-dusk on the next major version however as said earlier we need to revert this for now.

This PR should work great in some scenario however any interaction with elements where we need to use ElementResolver::findOrFail() etc will not work with the new syntax. A simple example is $browser->keys('@notes', 'Tests'); now failed.

@pataar
Copy link
Contributor Author

pataar commented Dec 16, 2022

We just reverted this because it was breaking Nova's test suite.

@driesvints @crynobone Any chance in bringing this back in the future?

@driesvints
Copy link
Member

@pataar mior answered this above your reply.

@pataar
Copy link
Contributor Author

pataar commented Dec 16, 2022

@pataar mior answered this above your reply.

@driesvints Alright, thanks!

@pataar
Copy link
Contributor Author

pataar commented Feb 2, 2023

This is now added by Taylor: cf04717

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.

5 participants