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

chore(deps): bump phlex to 2.0.0.rc #2

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

mhenrixon
Copy link
Contributor

@mhenrixon mhenrixon commented Dec 2, 2024

Let me know if this is of interest, it seems phlex 2.0 removes the deferred renderer but this should do almost the same thing. I have a couple of tweaks more but was hoping for some initial feedback so that I don't waste anyones time.

A couple of tests are not passing, there are warnings about onlick attribute not being safe but this should save you a bunch of figuring out.

@davidalejandroaguilar
Copy link
Contributor

Hey @mhenrixon, this is awesome! Thanks for taking the time to dig into this, truly a time saver 🙌🏼

I ran locally and things look good, a few things on the failed specs:

For the button_spec, I think we can:

  • Remove the specs that have the comment TODO: Not needed once Phlex 2.0 is released., since the failures are legit when using onclick without safe and the final HTML is properly escaped.
  • Fix the PhlexyUI::Button passing :modal option renders it correctly which only has spec attributes out of order, it's pretty harmless.

This should make the whole Button suite pass.

Other than that, I think it's only a matter of a bug in Phlex 2.0.0rc that is dasherizing attributes passed in as symbols with underscores, e.g. id: :my_id becomes id="my-id", I've opened an issue here phlex-ruby/phlex#827.

I think that should fix all failing specs!

Let's wait a bit for Joel to answer on that issue, see if it's going to be fixed or if we'd need to update the specs on this side.


Regarding the vanish changes, it's a bit mysterious, but hey it works! I'd probably add a comment on each usage explaining why we use it. I have asked Joel on bsky to see if that's going to be documented.

When you say "almost the same thing", what do you mean? Do you know some caveats?


Regarding the view helper, hopefully we'll see phlex-ruby/phlex#809 merged to get the Phlex::Testing::ViewHelper in without having to define it ourselves, but in the meantime, good to have it!


Thanks again! Excited to see people interested in Phlexy UI 😄.

Looking forward to the additional tweaks you mentioned you had in mind.

@davidalejandroaguilar
Copy link
Contributor

Ahh, from this discussion seems like Phlex 2.0 is dasherizing attributes by default, so we'd have to change our specs to pass the attribute as a string, e.g. "my_id".

@davidalejandroaguilar
Copy link
Contributor

Also, Joel answered on bsky. For those components that had Phlex::DeferredRender I think we should use the simpler form:

def view_template(&)
  yield(self) if block_given?
  # ...

Instead of:

def before_template(&)
  vanish(&)
  super
end

Since in the former, it's clearer what's happening and you don't have to wonder what that vanish method is doing.

Nice digging from you to find out about using vanish though!

@mhenrixon
Copy link
Contributor Author

A hundred percent with you and I'll update the code today

@mhenrixon
Copy link
Contributor Author

mhenrixon commented Dec 3, 2024

Hi @davidalejandroaguilar, thanks for your work on this. I love the library and am happy that I could contribute something ❤️

I think I addressed all the feedback and I remembered to check what standardrb was saying as well 🤣

@davidalejandroaguilar
Copy link
Contributor

davidalejandroaguilar commented Dec 3, 2024

Awesome, thank you, everything looks good! 🥇

Appreciate you taking the time to contribute and also the kind words 🙏🏼

Feel free to contribute again, there are plenty of components that’d be awesome to have. Otherwise, there are some components that need documentation, so you can always pick one of those if you want.

Thanks again!

@davidalejandroaguilar davidalejandroaguilar merged commit 75f6d1d into PhlexyUI:main Dec 3, 2024
1 check 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.

2 participants