-
Notifications
You must be signed in to change notification settings - Fork 292
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
base: master
Are you sure you want to change the base?
Conversation
@gmq @ldlsegovia I can't tell 100%, but I think the failing checks here are unrelated to my changes. Is that correct? |
Hi! It seems we need to downgrade node in our testing suite. In any case, a couple of notes:
|
cf91206
to
d41a210
Compare
@gmq thanks for the feedback. I addressed the first one by moving it into 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? |
@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? |
@gmq Can you please review this? |
@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
d41a210
to
5ff5c2c
Compare
@difernandez done. |
@dkniffin I took a look at this, and I have a couple of comments:
|
@difernandez Sorry, I've been busy. I will take a look at this again in the next two weeks or so. |
@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
This will produce a set of I will try to create an automated test for this in the next day or two. |
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? |
@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 |
The JS here is now scoping down the selector to the containing fieldset, rather than the whole document