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

Require Dusk selectors to adhere to CSS naming rules #1114

Closed

Conversation

pindab0ter
Copy link

Description

This PR fixes issue #1113 by refining the Dusk selector parsing to ensure compatibility with valid CSS class names. Currently, the documentation, PHPDoc, and unit tests do not clearly define which characters are valid for Dusk selectors, leading to potential conflicts when Dusk selectors are combined with other CSS selectors on the same element.

Motivation

The existing documentation and unit tests only uses dashes in Dusk selectors. However, Dusk selectors cannot be used alongside other CSS selectors if they target the same element, which requires them not to be separated by a space. By aligning the Dusk selector parsing with the standards for valid CSS names (as outlined in this Stack Overflow answer), we add support for Dusk selectors to target the same element as other CSS selectors.

Changes

  • Updated the Dusk selector parsing regex to only allow valid CSS names.
  • Modified the existing condition to use Str::contains instead of Str::startsWith to support Dusk selectors to exist after another CSS selector not separated by a space.
  • Added unit tests to ensure the new regex handles combined selectors correctly.

@pindab0ter pindab0ter changed the title Require Dusk selectors to adhere to the same limitations CSS class and ID names adhere to Require Dusk selectors to adhere to CSS naming rules Aug 8, 2024
@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions!

@pindab0ter
Copy link
Author

pindab0ter commented Aug 9, 2024

Can I get a response that goes into the core issue of the problem this is trying to solve?

This feels like a canned response, as this PR does only changes one line of code and adds two unit test cases.

@u01jmg3
Copy link
Contributor

u01jmg3 commented Aug 13, 2024

@pindab0ter: might be worth flagging to @driesvints ☮️

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