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

Set xpath flag if/when overwriting selector with previous_selector #278

Merged

Conversation

Matt-Yorkley
Copy link
Contributor

@Matt-Yorkley Matt-Yorkley commented May 30, 2023

Bug fix

Description

When chaining operations that use a selector argument, subsequent operations can optionally omit the selector argument and the selector from the previous operation gets applied applied automatically (at line 46 of CableReady::OperationBuilder here.

There's a weird edge case here that can cause the subsequent operation to fail if the first operation was using xpath, because the selector argument gets set but the xpath argument does not get set and defaults to false, eg:

cable_ready
  .add_css_class(name: "example", selector: "/html/body/div[1]", xpath: true) # succeeds
  .inner_html(html: "fail") # fails

The first operation is applied correctly but the inner_html operation fails to find it's element, as it has selector: "/html/body/div[1]", xpath: false, resulting in:

CableReady innerHtml operation failed due to missing DOM element for selector: '/html/body/div[1]`

The DOM element isn't missing, it's just not being looked for with xpath.

Why should this be added

Fixes an obscure edge case when chaining operations using xpath selectors and omitting the selector in subsequent operations.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

@netlify
Copy link

netlify bot commented May 30, 2023

Deploy Preview for cableready ready!

Name Link
🔨 Latest commit cf5094a
🔍 Latest deploy log https://app.netlify.com/sites/cableready/deploys/6476960b826bfd0008b548ca
😎 Deploy Preview https://deploy-preview-278--cableready.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@marcoroth marcoroth left a comment

Choose a reason for hiding this comment

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

Thank you for taking this one on @Matt-Yorkley!

lib/cable_ready/operation_builder.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@julianrubisch julianrubisch left a comment

Choose a reason for hiding this comment

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

LGTM 💚

Copy link
Contributor

@hopsoft hopsoft left a comment

Choose a reason for hiding this comment

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

NIce catch and fix. 👏🏻

lib/cable_ready/operation_builder.rb Show resolved Hide resolved
@julianrubisch julianrubisch merged commit 295c83b into stimulusreflex:main Jun 30, 2023
15 checks passed
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

4 participants