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

Fix html option settings for objects that have multiple values #386

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

gazayas
Copy link
Contributor

@gazayas gazayas commented Aug 8, 2023

I thought we would be ok with #317, but it turns out that merging it broke the tests.

What's happening here is that super select objects uses html_options: {multiple: true} whereas other objects just use multiple: true:

<!-- From PartialTest's _form.html.erb partial after running the Super Scaffolding test setup script -->

<%= render 'shared/fields/buttons', method: :multiple_buttons_test, multiple: true %>

<%= render 'shared/fields/super_select', method: :multiple_super_select_test, html_options: {multiple: true} %>

It's a little difficult to tell what's going on in this portion of the transformer, but I hope this comment helps to clear things up a little bit.

@gazayas gazayas marked this pull request as ready for review August 8, 2023 09:32
@jagthedrummer
Copy link
Contributor

jagthedrummer commented Aug 8, 2023

@gazayas Which tests did this break? CI was green on the main branch of core after I merged #317.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 9, 2023

@jagthedrummer Yes, I think we were getting a false positive in CI, and the Super Scaffolding partial tests were failing. I'm assuming the tests were running against released versions as opposed to the most recent changes on GitHub. I say that because I ran bin/hack --link in the starter repository against bullet_train-core's main branch and THEN ran the Super Scaffolding Partial tests which failed.

The other one failing right now is related to .daterangepicker. I'm hoping we can merge bullet-train-co/bullet_train#828 and #182, because I believe it will solve that issue as well. I saw the comment you left there about handling formats properly, so I'll try to work on that now.

@jagthedrummer
Copy link
Contributor

jagthedrummer commented Aug 9, 2023

@gazayas Ah-ha! It all makes sense now. It's what I found in bullet-train-co/bullet_train#831. We were looking at the same thing, just coming at it from different directions.

So, just kinda thinking out loud here. Does it seem reasonable to do the following?

  1. Merge this PR so that tests go green in Don't display header if no title or description #831.
  2. Prepare a "revert" for this PR so we can get these changes back when we need them.
  3. Merge Don't display header if no title or description #831.
  4. Deal with Update dates helper bullet_train#828 & Standardize Date and DateTime locales #182 (being able to see their test status in the starter repo against both the released gems and core/main).
  5. Merge the revert for this PR.

Also open to suggestions!

@jagthedrummer
Copy link
Contributor

Since we can always revert things if we need to, and in the interest of getting PRs closed, I'm going to proceed with the plan I mentioned above.

@jagthedrummer jagthedrummer merged commit bd547b2 into main Aug 9, 2023
@jagthedrummer jagthedrummer deleted the fixes/scaffolding-multiple-options branch August 9, 2023 17:19
@jagthedrummer
Copy link
Contributor

Welp, merging this didn't fix the situation in bullet-train-co/bullet_train#831. Now I'm suspecting that #334 has something to do with it. Unfortunately too much stuff has changed since we merged that and we can't just generate a revert PR for it. I'm going to try making a branch and undoing stuff in that PR by hand.

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