search

PhlexUI / phlex_ui

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

Add some very superficial tests #39

Closed oneiros closed 2 months ago

oneiros commented 2 months ago

As I wrote in #38 I also started looking into phlex's deprecation of the #template method. And I also stumbled upon the missing tests to somehow verify the warning is gone but components still render.

So I wanted to add some very simple tests. They are mostly auto-generated and then adjusted manually where necessary.

The tests only try to render the components using defaults where possible or very minimal test parameters where not. Nothing fancy and barely useful, but hopefully better than nothing.

I stumbled upon two problems I had to solve:

  1. Turns out phlex_ui implicitly depends on the json and activesupport gems. It uses #to_json in some places and a couple of AS core extensions (Object#present?, String#underscore and String#html_safe). I made these dependencies explicit. Of course, AS is a heavy dependency and it might make sense to think about removing it. But for now I made sure to require only what is needed. And in rails apps it is always there anyway.
  2. I think the tests surfaced a little bug: PhlexUI::Select::Content had a default value of nil for the outlet_id parameter, but then goes on to call methods on the value that nil does not have. My fix was simply to remove the default.

I did not go so far as to add a Github action to run these test automatically. If this is merged, it might make sense to add one. (I might even look into it, but maybe someone here is a little bit more comfortable with GH actions than me and beats me to it :slightly_smiling_face: )

iseth commented 2 months ago

Great PR. I think it would be best to merge this and #38 to update-phlex-22 and test things in that branch before we merge things into main.

Also I would like to start adding support for sinatra and other making this project not completely rails dependent so good catch on the places active support is being used. I don't think we should explicitly add them as requirements to the gemspec but instead document which calls are AS like you did and remove them in the next release preparing for support for sinatra and not making it explicitly rails.

mrinterweb commented 2 months ago

This looks like a good starting place for tests. I'd say fix up those style format warnings to make CI happy.

iseth commented 2 months ago

thats something i am fixing on main right now. Its with an updated version of standard. @mrinterweb