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

Add :by_module option to :types to allow passing in the embedded_struct in the type_field. #65

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Ivor
Copy link

@Ivor Ivor commented Oct 5, 2022

This PR is based on the idea and work from https://github.com/pzingg/polymorphic_embed/tree/type-module-fun referenced in #18

To make for the smallest change, I have only added the :by_module option. Please let me know if I can make this better or if I have missed anything. The tests that I added pass, but I am not sure whether I might be overlooking something.

@mathieuprog
Copy link
Owner

Wow. I need some time though, can you depend on your branch for now?

Can you explain your use case please?

@mathieuprog mathieuprog added the enhancement New feature or request label Oct 5, 2022
@Ivor
Copy link
Author

Ivor commented Oct 5, 2022

Hi, the use case we have is a centralised way of capturing notes. This is used by different dashboards for different industries. Each industry has different extra fields to capture. So different contexts want to capture different nested structures. The Notes don't know about the different dashboards so I can't pass the keyword list.

I can use my fork and see how it pans out. If there are any specific short comings please let me know and I can learn how to fix those.

@mathieuprog
Copy link
Owner

Did you try it on your project already? Does it cover all of your use case that you described?
If you don't know yet, when will you be able to share some feedback?

@Ivor
Copy link
Author

Ivor commented Oct 5, 2022

I haven't tried it. Just added the tests here.
We use the current version with hard-coded types.
I can implement it and provide feedback by Wednesday next week. Maybe sooner if I have something to report.
Thanks for the quick response!

@Ivor
Copy link
Author

Ivor commented Oct 6, 2022

@mathieuprog I added tests for most of the remaining cases. In doing the tests I realise my previous attempt did not quite have everything that is needed.

I tried in our project but we're using Surface and at this point it feels like a bridge too far getting the changesets to work properly with Surface and Phoenix forms. One problem is that when inputs_for needs to get the fields from the polymorphic embed, with the :by_module option the information is simply not there.

I do not have the time to dive deep enough into the forms to get that working. In our case we can build the form without needing inputs_for since the parent record is mostly a container for some metadata around our notes.

I will leave this here in case someone else has the opportunity to delve into the changesets and forms and inputs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants