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 issue with has_many nested_input #437

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dkniffin
Copy link
Contributor

The JS here is now scoping down the selector to the containing fieldset, rather than the whole document

@dkniffin
Copy link
Contributor Author

@gmq @ldlsegovia I can't tell 100%, but I think the failing checks here are unrelated to my changes. Is that correct?

@gmq
Copy link
Member

gmq commented Dec 20, 2022

Hi! It seems we need to downgrade node in our testing suite.

In any case, a couple of notes:

  • all.js is auto-generated, to make the change you need to do it in app/javascript/activeadmin_addons/inputs/nested-select.js
  • We're working on a refactor to remove select2 (since it's problematic when using modern bundlers like esbuild) so maybe wait until that's done to avoid spending time in something that'll be replaced.

@dkniffin
Copy link
Contributor Author

@gmq thanks for the feedback.

I addressed the first one by moving it into nested-select.js

Regarding the second point, is there going to be a replacement to continue supporting nested selects? Also, what kind of eta is there on that refactor?

@dkniffin
Copy link
Contributor Author

@gmq bump. I'd like to get this fixed, as it's causing my users some issues. If I move these code changes to the right place, would it be possible to get this merged?

@dkniffin
Copy link
Contributor Author

@gmq Can you please review this?

@difernandez
Copy link
Contributor

@dkniffin could you please rebase this from master? It appears the tests failed here because of an issue that is now fixed in master. Then we can review this with passing tests

The JS here is now scoping down the selector to the containing fieldset, rather than the whole document
@dkniffin
Copy link
Contributor Author

@difernandez done.

@difernandez
Copy link
Contributor

@dkniffin I took a look at this, and I have a couple of comments:

  • I'm not sure I completely understand the use case that is being fixed here. Please provide an issue number, or update the description of this PR to include a more detailed explanation, ideally with an example to reproduce the problem
  • Please provide a test case for the issue being solved, to make sure the fix is working, and it also would serve as an example
  • I think using the fieldset.inputs selector could be breaking for users that for some reason didn't include a f.inputs in their form definition (probably uncommon, but not technically required). Maybe using a form selector? Or some other way to navigate this that doesn't require the f.inputs

@dkniffin
Copy link
Contributor Author

@difernandez Sorry, I've been busy. I will take a look at this again in the next two weeks or so.

@dkniffin
Copy link
Contributor Author

@difernandez Alright, finally got around to looping back.

Here's an example use-case I'm trying to solve here. This is copy/pasted exactly from my code, with model names and attributes obscured as foo, bar and baz

              f.has_many :foos, allow_destroy: true, new_record: "Add Foo" do |p|
                p.input :bar_id, as: :nested_select,
                  level_1: {
                    attribute: :baz_id,
                    collection: bazzes
                  },
                  level_2: {
                    attribute: :bar_id,
                    display_name: :title,
                    collection: bars,
                    required: false,
                    method_model: Bar
                  }

This will produce a set of has_many sections, where each section has a nested dropdown. The expected behavior is: when you select a "Baz", the dropdown for the related "Bar" should reset/update accordingly. However, what actually happens is all "Bar" dropdowns reset/update.

I will try to create an automated test for this in the next day or two.

@dkniffin
Copy link
Contributor Author

Actually, it looks like this may be resolved by the switch to slim-select on the 2.0 beta. There are some other issues though. I'll see if I can resolve those.

@difernandez Is there an eta on when 2.0 will be released?

@difernandez
Copy link
Contributor

@dkniffin unfortunately we don't have an ETA on getting v2 out of beta. It should be stable as it is, but we want to test it for a while, see if we find any major issues, just in case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants