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

Bug: cannot analyse value if is interpolated #61

Merged
merged 10 commits into from
Nov 23, 2023

Conversation

Gert-JanPeeters
Copy link
Contributor

@Gert-JanPeeters Gert-JanPeeters commented Oct 26, 2023

I ran into the same issue as described in #21 and #28.

This also supports blade and php, next to erb.

ToDo:

  • data-controller
  • data-[identifier]-[value]-value
  • data-[identifier]-target
  • data-action
  • cleanup

Copy link
Owner

@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.

Thanks for working on this @Gert-JanPeeters! I think this resolves the issues at hand.

Though, there are some cases we should think about. I'm not sure if this should also happen in this PR, but here it goes:

  • Currently this only fixes it for data-[identifier]-[value]-value attributes, but technically this could be the case for any Stimulus-attribute.

    So maybe it would make sense to extract this logic out of the validateDataValueAttribute() function and apply it for all the data attribute validation methods in the diagnostics.ts file.

  • With the current approach we skip the analysis of the whole attribute if it finds any kind of interpolation, but I assume there are also cases like this:

 <div data-controller="some-controller <%= "some-dynamic-controller" %>">
   <!-- ... -->
 </div>

I guess we could ignore this for now and open another issue for it. But I think we should still try to process the analysis for some-controller in this case.

What do you think?

@Gert-JanPeeters
Copy link
Contributor Author

Gert-JanPeeters commented Oct 26, 2023

I think we should indeed tackle the issues you raised. No point in postponing it to the future.

Let me work on this a bit more so we also resolve the other issues that you raised 👍🏼

@Gert-JanPeeters Gert-JanPeeters marked this pull request as draft November 1, 2023 07:36
@Gert-JanPeeters Gert-JanPeeters marked this pull request as ready for review November 6, 2023 19:25
Copy link
Owner

@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.

This looks great to me @Gert-JanPeeters, thanks! 🙌🏼

We can merge it if you could resolve the merge conflicts

@Gert-JanPeeters
Copy link
Contributor Author

Ready to be merged 🚀

@marcoroth marcoroth merged commit 9e5fc8e into marcoroth:main Nov 23, 2023
@marcoroth
Copy link
Owner

Thank you so much @Gert-JanPeeters! 🙌🏼

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