search

PhlexUI / phlex_ui

Ruby gem for Phlex UI Components
https://phlexui.com/
MIT License
115 stars 11 forks source link

Renaming `template` method to `view_template` #38

Closed mrinterweb closed 2 months ago

mrinterweb commented 2 months ago

When opening the rails console, I would get the following message 148 times:

⚠️ [DEPRECATION] Defining the template method on a Phlex component will not be supported in Phlex 2.0. Please rename the method to view_template instead.

I renamed all of these with the following command:

rg -l "def template" | xargs -I@ sed -i '' 's/def template/def view_template/g' @
oneiros commented 2 months ago

I was looking into this as well. You beat me to it :slightly_smiling_face:

Well done!

There is a little detail though, that I think is missing: The deprecation warning arrived with phlex 1.10 to give a heads up that template has been renamed and will stop working in 2.0. You can rename to view_template now and get rid of the warnings, but this will not work with older versions of phlex.

So I think the minimum version of phlex in the gemspec here needs to be raised to 1.10 for this change here to not break anything.

iseth commented 2 months ago

Great PR! Yeah good point @oneiros

iseth commented 2 months ago

Yeah if we can get that genspec updated I’ll merge this PR

mrinterweb commented 2 months ago

I don't have a means of testing this MR. I don't see specs/tests in the gem, and I don't have much that I can do for local testing. The only testing I've done is check that running rails c no longer produces the warning. It would be good if someone who knows how to test this change could smoke test this to make sure it works as I expect it does.

oneiros commented 2 months ago

I don't see specs/tests in the gem

I tried to add some really superficial ones here: #39. Maybe that helps.

iseth commented 2 months ago

This looks great! If things look good to you I'll merge it into update-phlex-22 and add the tests #39 there too and do some more testing and any final modifications before merging that branch to main.

mrinterweb commented 2 months ago

@iseth that sounds good. Any way this change gets upstreamed is good with me. Feel free to merge this branch to whatever branch you'd like.