Update field partials for correct use of options variables #633

gazayas · 2023-10-25T09:46:15Z

Closes #631.

The current state of this PR doesn't change the use of options and html_options, but it does remove them from render calls where they aren't necessary. I've added comments to those invocations to make the PR easier to review, but we can delete them later if we decide we don't need them.

Add docs for options vs html_options
Write comments in the code for clarity
Remove options or html_options where necessary in bullet_train-themes partials
Remove options or html_options where necessary in bullet_train-themes-tailwind-css partials
Update Scaffolding::Transformer to scaffold options correctly if necessary
Change all instances of other_options to bt_options
Add any deprecation messages where necessary
Check showcase partials if they need to be updated
Add a page to the documentation for updating to the next version

There are a lot of tasks here, so I'm okay with breaking down this one into pieces if we need to.

_field.html.erb

These two files are seemingly the same, so I wonder if we can consolidate this all into one file.

EDIT:
The tailwind_css field partial was a duplicate, so I deleted it here f3fdcee

gazayas · 2023-10-25T10:36:37Z

I went ahead and deleted bullet_train-themes-tailwind_css/app/views/themes/tailwind_css/fields/_field.html.erb, it looks like we didn't actually need it.

gazayas · 2023-11-04T04:28:30Z

We'll have to look over this block to make sure we're scaffolding multiple: true correctly.

bullet_train-core/bullet_train-super_scaffolding/lib/scaffolding/transformer.rb

Lines 739 to 747 in cdccbdd

    
           # When rendering a super_select element we need to use `html_options: {multiple: true}`, 
        
           # but all other fields simply use `multiple: true` to work. 
        
           if attribute.is_multiple? 
        
             if attribute.type == "super_select" 
        
               field_options[:multiple] = "true" 
        
             else 
        
               field_attributes[:multiple] = "true" 
        
             end 
        
           end

EDIT:
Updated in 00dcfe3

gazayas · 2023-11-04T05:28:06Z

Still a handful of partials to work through, but I'm hoping to get this one over the finish line sometime next week. I'm foreseeing the following partials being a little difficult to adjust:

_cloudinary_image.html.erb
_date_and_time_field.html.erb
_date_field.html.erb
_option.html.erb
_options.html.erb

gazayas · 2023-11-06T12:10:41Z

Managed to get the cloudinary image partial and color picker partial out of the way today, only a few more partials left.

One thing to note is that color picker options were being housed like this:
options[:color_picker_options]

Other places, like the button partial, simply put the array of values into options directly (without a key). I've decided to just use an array named color_picker_field_options. This is the same as what I've done with button_field_options. I hope to unify all of these with the *_field_options naming convention.

gazayas · 2023-11-08T12:00:31Z

@jagthedrummer This PR is done for the most part, so I marked it for review as I think over how to add documentation for upgrading to the next version. I know you did a lot of upgrading docs before, so any insight would be helpful.

I also have left the other_options to bt_options task unchecked for now because there's already a lot going on here. Do you think it should be handled in this PR as well, or should we separate it?

Besides the docs, I'll look over things one more time to see what I can clean up.

jagthedrummer · 2023-11-10T19:16:37Z

I've been pretty tied up this week and haven't had a chance to look at this. I'll give it a good review next week. Thanks for taking this on, @gazayas!

gazayas · 2023-11-11T06:42:19Z

@jagthedrummer No problem at all! Just as a point of reference, here is the main method that calls the Rails form helpers, which is what I think we should base options and html_options off of.

bullet_train-core/bullet_train-themes-light/app/views/themes/light/fields/_field.html.erb

Lines 69 to 81 in 6d2ca8a

    
             <%# Here, we prioritize yielding the partial's field markup if it already exists. %> 
        
             <%# `form.send` below calls the original Rails Form helpers, such as %> 
        
             <%# `form.text_field(method, options)`. Refer to the form helpers for more details: %> 
        
             <%# https://github.com/rails/rails/blob/main/actionview/lib/action_view/helpers/form_helper.rb %> 
        
             <div class="mt-1.5"> 
        
               <% # The actual field. %> 
        
               <% if partial.field? %> 
        
                 <%= partial.field %> 
        
               <% else %> 
        
                 <% # e.g. form.text_field(method, options) %> 
        
                 <%= form.send(helper, method, options) %> 
        
               <% end %> 
        
             </div>

gazayas · 2023-11-11T07:02:21Z

bullet_train-super_scaffolding/lib/scaffolding/transformer.rb

@@ -754,7 +742,7 @@ def valid_#{attribute.collection_name}

        # https://stackoverflow.com/questions/21582464/is-there-a-ruby-hashto-s-equivalent-for-the-new-hash-syntax
        if field_options.any? || options.any?
-          field_options_key = if ["buttons", "super_select", "options"].include?(attribute.type)


Super Select is the only partial that uses html_options under the hood, whereas buttons and options simply use options (all the BT-side invocations in this comment are taken from main).

Super Select:

select API and BT-side invocation

Buttons:

check_box API and BT-side invocation

radio_button API and BT-side invocation

Options:

check_box and BT-side invocation

check_box_tag and BT-side invocation

radio_button and BT-side invocation

gazayas · 2023-11-11T08:16:06Z

bullet_train-themes-light/app/views/themes/light/fields/_field.html.erb

+# TODO: Partials which invoke form helpers inline aren't using
+# these classes, although we have the `has_errors` conditional with some style below.
 options[:class] = "#{options[:class]} block w-full rounded-md shadow-sm font-light text-sm dark:bg-slate-800 dark:text-slate-300"
-
 options[:class] += if has_errors
  " pr-10 border-red-500 text-red-900 placeholder-red-500 outline-none focus:ring-red-500 focus:border-red-500 dark:text-slate-300"
 else
  " focus:ring-primary-500 focus:border-primary-500 border-slate-300 dark:border-slate-900"
 end


Example of the proper classes not being added to a date_and_time_field named foo although we're passing options to render 'shared/fields/field'. I'm thinking of making a helper to compensate for this.

gazayas · 2023-11-11T08:45:29Z

I've only edited comments for these last three commits and the tests started to fail, it looks like the Open API tests are acting up again.

EDIT:
One more thing I did to make sure tests were passing:

Run the Super Scaffolding test setup script in the starter repository against the main branch of bullet_train-core
After the setup, I switched the gems to my local copy of bullet_train-core and tested with that, and the tests passed (specifically test/system/super_scaffolding_test.rb and test/system/super_scaffolding_partial_test.rb).

gazayas · 2023-11-11T10:10:18Z

One more note before I call it a day. I decided to pass all options to render 'shared/fields/field' for now, but I think we can remove them in the future. It's needed right now for two reasons:

For date-related partials to work (it only needs options[:id] though, not the whole hash).
To add specific classes to the form element, but this is only working for partials that invoke form.send(helper, method, options) (details here).

I think we can find a way around these two issues and eventually remove options: options from render 'shared/fields/field' altogether. However, we don't have to worry about it for the scope of this PR.

jagthedrummer · 2023-11-17T16:18:38Z

Just a note for myself that we want to wait to merge this until the following things have happened:

The fix mentioned above released in a new rails version
The starter repo updated to that new version

Update: The fix was release in 7.1.3 and the starter repo has been updated.

gazayas · 2023-11-21T10:47:50Z

I added some more documentation explaining options and other_options in more detail, along with the general other_options that are available for all of the field partials. I think this information will be useful for developers so they know exactly where their information is going (a.k.a what form helper is being used), and we can use it as a guideline in the future in case we're not sure what options or values we want to pass to partials on the Bullet Train side.

jagthedrummer · 2024-01-18T19:09:33Z

@gazayas it looks like the rails fix that we were waiting on was released a couple of days ago in 7.1.3 and the starter repo has already been updated (thanks, depfu!). So I think we can pick this one back up. Would you mind looking at the merge conflict when you have some time?

gazayas · 2024-01-19T14:02:47Z

@jagthedrummer It's been a hot minute so there are going to be a lot of commits from the merge here, but the only conflict was from the update in #742 so it shouldn't be too bad. Also, super scaffolding system tests passed locally for me.

gazayas · 2024-01-19T14:04:16Z

Ah, it just consolidated everything into the merge: 54518e4

Alright, good to go!

gazayas · 2024-01-19T14:04:36Z

Linter...

gazayas · 2024-01-19T14:07:30Z

Hmm, the linter's still failing, I'll need to look at this one in a little more depth.

gazayas · 2024-01-19T14:11:51Z

Looks like a Ruby version issue. I'll have to swing back to this one in a little bit, will ping you again soon:

bullet_train-core/.github/workflows/_standardrb.yml

Line 25 in 1a4ed61

ruby-version: "3.2"

jagthedrummer · 2024-01-19T15:46:02Z

@gazayas I'm working on the ruby version problem. Will merge this as soon as I have the kinks worked out. #748

gazayas · 2024-01-23T08:41:21Z

@jagthedrummer I didn't tag you earlier when I merged the latest changes with main, just let me know if you need any more changes before merging!

jagthedrummer · 2024-03-18T18:59:41Z

@gazayas, I'm so sorry that I missed your update on this a couple of months ago. It looks like we have some more conflicts now. 😞

For some reason GitHub isn't letting me resolve them in the browser, so I just took a stab at rebasing all of this locally. I pushed it to a new branch and am letting CI run. https://github.com/bullet-train-co/bullet_train-core/pull/799/files

If that PR looks right (and if CI passes) I can push that branch onto this PR.

Sorry that I let this hang out so long. 🤦

gazayas · 2024-03-22T06:55:47Z

@jagthedrummer No problem! I just merged main again and the conflicts weren't so bad (mainly just comments I added here which weren't on main).

If you still want me to look at the rebase PR you submitted, I'd be glad to look at it.

jagthedrummer · 2024-03-22T21:18:12Z

Nah, don't worry about that other PR @gazayas. I'll go ahead and close that one now, and then this PR will be on the top of my list to get merged next week. Thanks again!

jagthedrummer · 2024-03-29T15:11:53Z

OK, @gazayas I just did some testing with this and we're getting really close! Here's what I did to test:

First I had my starter repo using the released versions of all the gems, and then I generated a Project model with a bunch of fields to test against.

bin/super-scaffold crud Project Team \
  name:text_field \
  logo:file_field \
  address:address_field \
  a_buttons:buttons \
  some_buttons:buttons{multiple} \
  a_color:color_picker \
  a_date_with_time:date_and_time_field \
  a_date:date_field an_option:options \
  some_options:options{multiple} \
  trix_editor_area:trix_editor \
  a_password:password_field \
  phone_number:phone_field \
  a_super_select:super_select \
  some_super_selects:super_select{multiple}

Then I ran bin/hack --link ../bullet_train-core to link my starter repo to this branch in my core repo.

And then I visited the form for editing a project, and I see two pairs (four total) deprecation messages in the log:

DEPRECATION WARNING: The `multiple` attribute will be removed in a later version.
  Please pass `options: {multiple: true}` to `render` instead.
  (called from render at /Users/jgreen/projects/bullet-train-co/bullet_train-core/bullet_train-themes/app/helpers/current_theme_helper.rb:19)
DEPRECATION WARNING: Calling warn on ActiveSupport::Deprecation is deprecated and will be removed from Rails
  (use your own Deprecation object instead)
  (called from ___sers_jgreen_projects_bullet_train_co_bullet_train_core_bullet_train_themes_tailwind_css_app_views_themes_tailwind_css_fields__buttons_html_erb__2608134009605983127_119560 at /Users/jgreen/projects/bullet-train-co/bullet_train-core/bullet_train-themes-tailwind_css/app/views/themes/tailwind_css/fields/_buttons.html.erb:38)


DEPRECATION WARNING: The `multiple` attribute will be removed in a later version.
  Please pass `options: {multiple: true}` to `render` instead.
  (called from render at /Users/jgreen/projects/bullet-train-co/bullet_train-core/bullet_train-themes/app/helpers/current_theme_helper.rb:19)
DEPRECATION WARNING: Calling warn on ActiveSupport::Deprecation is deprecated and will be removed from Rails
  (use your own Deprecation object instead)
  (called from ___sers_jgreen_projects_bullet_train_co_bullet_train_core_bullet_train_themes_tailwind_css_app_views_themes_tailwind_css_fields__options_html_erb___67058703469920398_119680 at /Users/jgreen/projects/bullet-train-co/bullet_train-core/bullet_train-themes-tailwind_css/app/views/themes/tailwind_css/fields/_options.html.erb:36)

As happened previously the warnings point to theme_helper.rb instead of to my project form due to bullet_train-themes being a local file. So I set my Gemfile to use the released version of bullet_train-themes.

After doing that the deprecation messages look like this:

DEPRECATION WARNING: The `multiple` attribute will be removed in a later version.
  Please pass `options: {multiple: true}` to `render` instead.
  (called from block (3 levels) in _app_views_account_projects__form_html_erb__4604713326196375126_119220 at /Users/jgreen/projects/bullet-train-co/bullet_train/app/views/account/projects/_form.html.erb:12)
DEPRECATION WARNING: Calling warn on ActiveSupport::Deprecation is deprecated and will be removed from Rails
  (use your own Deprecation object instead)
  (called from ___sers_jgreen_projects_bullet_train_co_bullet_train_core_bullet_train_themes_tailwind_css_app_views_themes_tailwind_css_fields__buttons_html_erb__956510538507361194_119580 at /Users/jgreen/projects/bullet-train-co/bullet_train-core/bullet_train-themes-tailwind_css/app/views/themes/tailwind_css/fields/_buttons.html.erb:38)



DEPRECATION WARNING: The `multiple` attribute will be removed in a later version.
  Please pass `options: {multiple: true}` to `render` instead.
  (called from block (3 levels) in _app_views_account_projects__form_html_erb__4604713326196375126_119220 at /Users/jgreen/projects/bullet-train-co/bullet_train/app/views/account/projects/_form.html.erb:17)
DEPRECATION WARNING: Calling warn on ActiveSupport::Deprecation is deprecated and will be removed from Rails
  (use your own Deprecation object instead)
  (called from ___sers_jgreen_projects_bullet_train_co_bullet_train_core_bullet_train_themes_tailwind_css_app_views_themes_tailwind_css_fields__options_html_erb___997636023711715181_119700 at /Users/jgreen/projects/bullet-train-co/bullet_train-core/bullet_train-themes-tailwind_css/app/views/themes/tailwind_css/fields/_options.html.erb:36)

So the deprecation warnings that we're raising are now being reported from the right place (app/views/account/projects/_form.html.erb), which is great! But there's an additional deprecation warning that we are not raising, but is being raised by rails because of the way that we're raising our own deprecation warning. (Yo dawg, we heard you like deprecation warnings... 😜)

Looks like instead of doing this:

ActiveSupport::Deprecation.warn("our message")

We should do something more like this:

ActiveSupport::Deprecation.new.warn("our message")

jagthedrummer · 2024-03-29T15:26:09Z

I just made the change mentioned above. Gonna let CI run, and then merge this sucker. Thanks for your work on this @gazayas! It's definitely going to make the whole options thing more consistent and therefore hopefully less confusing.

gazayas · 2024-03-30T01:07:33Z

(Yo dawg, we heard you like deprecation warnings... 😜)
😆

Thanks for picking up that last part on the deprecation warnings! I saw that there's a merge conflict (?) on #801 so I'll take a glance at that one, glad this one is merged though!

In #633 we standardized the way that we pass various options to various field partials. It looks like we had some confusion about whether we were using `buttion_field_options` or `button_field_buttons` as the new variable name. We were using both in difference spots, but they didn't agree so it just didn't work. This PR standardizes on `button_field_options` to match the pattern that we're using in other field partials. It removes `button_field_buttons`. Fixes #815

gazayas mentioned this pull request Nov 7, 2023

Date-related partials generate duplicate IDs #649

Closed

gazayas marked this pull request as ready for review November 8, 2023 12:00

gazayas commented Nov 11, 2023

View reviewed changes

gazayas added 17 commits November 13, 2023 11:00

Add comment in field partial concerning helper invocations

5ca6e28

Write details concerning options in _field partial

8216e95

Update Field Partial docs: options vs. html_options

f09c83c

Update themes fields to reflect proper use of options and html_options

ab13692

Delete potentially unnecessary duplicate partial

1a001e0

Update use of options in active_storage_image partial

393e050

Update active_storage_image partial

e439411

Comment out unneeded variables in address partial

0a31d40

Update buttons partial

91c1220

Deprecate use of options in buttons partial

4d63ad4

Initialize button_field_buttons in buttons partial

a72e03d

Ensure options is a hash in buttons partial

a993386

Actually reinitialize options

ad2b748

Remove unneeded ID assignment in buttons partial

1bddfc2

Fix typo

96f28f4

Update cloudinary image partial

564c104

Update color picker partial and the transformer logic behind it

4eeafc6

Add documentation for options and other_options

b04acb8

Merge branch 'main' into fixes/update-options

54518e4

Fixing Standard Ruby

f98588d

Merge branch 'main' into fixes/update-options

dfbf678

jagthedrummer mentioned this pull request Mar 18, 2024

Fixes/update options rebased #799

Closed

Merge branch 'main' into fixes/update-options

6a556a4

Instantiate a deprecator before calling warn

883ecc8

jagthedrummer approved these changes Mar 29, 2024

View reviewed changes

jagthedrummer merged commit 567d4ad into main Mar 29, 2024
30 checks passed

jagthedrummer deleted the fixes/update-options branch March 29, 2024 16:00

jagthedrummer mentioned this pull request Mar 29, 2024

Custom http methods for API documentation with relevant examples #801

Merged

pascallaliberte mentioned this pull request Apr 4, 2024

Fix Action Models select multiple no longer working bullet-train-pro/bullet_train-action_models#101

Merged

jagthedrummer mentioned this pull request May 6, 2024

Bug fix for passing options to the buttons field partial #824

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update field partials for correct use of options variables #633

Update field partials for correct use of options variables #633

gazayas commented Oct 25, 2023 •

edited

Loading

gazayas commented Oct 25, 2023

gazayas commented Nov 4, 2023 •

edited

Loading

gazayas commented Nov 4, 2023

gazayas commented Nov 6, 2023

gazayas commented Nov 8, 2023

jagthedrummer commented Nov 10, 2023

gazayas commented Nov 11, 2023

gazayas Nov 11, 2023 •

edited

Loading

gazayas Nov 11, 2023

gazayas commented Nov 11, 2023 •

edited

Loading

gazayas commented Nov 11, 2023

jagthedrummer commented Nov 17, 2023 •

edited

Loading

gazayas commented Nov 21, 2023

jagthedrummer commented Jan 18, 2024

gazayas commented Jan 19, 2024

gazayas commented Jan 19, 2024

gazayas commented Jan 19, 2024

gazayas commented Jan 19, 2024

gazayas commented Jan 19, 2024

jagthedrummer commented Jan 19, 2024

gazayas commented Jan 23, 2024

jagthedrummer commented Mar 18, 2024

gazayas commented Mar 22, 2024

jagthedrummer commented Mar 22, 2024

jagthedrummer commented Mar 29, 2024

jagthedrummer commented Mar 29, 2024

gazayas commented Mar 30, 2024

Update field partials for correct use of options variables #633

Update field partials for correct use of options variables #633

Conversation

gazayas commented Oct 25, 2023 • edited Loading

_field.html.erb

gazayas commented Oct 25, 2023

gazayas commented Nov 4, 2023 • edited Loading

gazayas commented Nov 4, 2023

gazayas commented Nov 6, 2023

gazayas commented Nov 8, 2023

jagthedrummer commented Nov 10, 2023

gazayas commented Nov 11, 2023

gazayas Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

gazayas Nov 11, 2023

Choose a reason for hiding this comment

gazayas commented Nov 11, 2023 • edited Loading

gazayas commented Nov 11, 2023

jagthedrummer commented Nov 17, 2023 • edited Loading

gazayas commented Nov 21, 2023

jagthedrummer commented Jan 18, 2024

gazayas commented Jan 19, 2024

gazayas commented Jan 19, 2024

gazayas commented Jan 19, 2024

gazayas commented Jan 19, 2024

gazayas commented Jan 19, 2024

jagthedrummer commented Jan 19, 2024

gazayas commented Jan 23, 2024

jagthedrummer commented Mar 18, 2024

gazayas commented Mar 22, 2024

jagthedrummer commented Mar 22, 2024

jagthedrummer commented Mar 29, 2024

jagthedrummer commented Mar 29, 2024

gazayas commented Mar 30, 2024

gazayas commented Oct 25, 2023 •

edited

Loading

gazayas commented Nov 4, 2023 •

edited

Loading

gazayas Nov 11, 2023 •

edited

Loading

gazayas commented Nov 11, 2023 •

edited

Loading

jagthedrummer commented Nov 17, 2023 •

edited

Loading