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

repeatable_attr_input's simple_form_input_args[:label] option should be respected by add_another_text method #162

Open
ewlarson opened this issue Mar 17, 2023 · 9 comments

Comments

@ewlarson
Copy link
Contributor

It would be nice if repeatable_attr_input kept this potential label arg in mind when ultimately generating the add_another_text value. Otherwise, you end up with a nice label and a funny "Add another Gbl displaynote sm" link:

Screenshot 2023-03-17 at 11 13 25 AM

I know you can add these field strings into a I18n file, but I specifically don't want to have to do that. I'm pulling all of my form fields, field types, labels, html_attributes, etc. from an ActiveRecord model:
https://github.com/geobtaa/geomg/blob/feature/attribute-model/db/schema.rb#L238-L264

Hoping to find time this week or next for a little PR to support this...

@jrochkind
Copy link
Contributor

jrochkind commented Mar 17, 2023

I'm not completely following where else it would get it from -- can't you already pass in an arg? Or no, there's no arg? I18n or an arg you pass in to repeatable_attr_input (or guessing from model name)-- where else could it get it from? Or am I wrong it's got an arg? Regardless, happy to look at PR!

@ewlarson
Copy link
Contributor Author

Sorry to confuse! Basically, this code:

<%= f.repeatable_attr_input element.solr_field.to_sym, build: :at_least_one, html_attributes: options[:html_attributes], simple_form_input_args: { label: element.label } %>

Produces this result
Screenshot 2023-03-17 at 11 13 25 AM

Where the field's label is "Display Note" (my element.label value), but the add another link value doesn't use that same label value. Instead it looks to I18n to find a match, and misses, ultimately rendering "Add another Gbl displaynote sm"

def add_another_text
if base_model.class.respond_to?(:human_attribute_name)
I18n.t("kithe.repeatable_input.add_a", name: base_model.class.human_attribute_name(attribute_name))
elsif repeatable_type_class.respond_to?(:model_name)
I18n.t("kithe.repeatable_input.add_a", name: attribute_model_class.model_name.human)
else
I8n.t("kithe.repeatable_input.add_bare")
end
end

Would prefer it to render "Add another Display Note"

@jrochkind
Copy link
Contributor

OH I see your title now, I wans't paying enough attention. OK, I think it sounds like a bug?

@ewlarson
Copy link
Contributor Author

ewlarson commented Mar 17, 2023

Not so much a bug, but an oversight? Also, I18n would work great for everyone, except me pulling fields out of a database table.

@jrochkind
Copy link
Contributor

Oh wait, you are just passing it in as simple_form_input_args:... sorry I'm not paying enough attention and refershing myself as to context.

I don't think attr_json paying attention to what are essentially private simple_form_input_args is the right move necessarily....

hmm. Looking at the args to the method... I wonder if it needs a new arg. Not sure.

@ewlarson
Copy link
Contributor Author

If you pass in a field label, the add another link should use that same label. This isn't an attr_json issue, but an issue with the cocoon(-ish) repeatable element rendering code.

@jrochkind
Copy link
Contributor

OK! Thanks for bringing this up @ewlarson! I honestly have some other stuff I'm bogged down in at the moment, if you want to do a PR that would be awesome, although I might end up having Ideas about it.

I'm still cautious about "reaching into" the arguments at simple_form_input_args -- i see those meant to be a "black box" to be passed to simple_form, I don't like the idea of looking into there and using them for other things. I think the best solution is probably elsewhere. But curious to share ideas!

While I'm a bit bogged down now, I might have more time in a couple weeks to look at it myself, have a zoom conversation/code share with you about it, or whatever. Thank you!

@ewlarson
Copy link
Contributor Author

Thanks for the comment!

Yeah, I just think the field label and the repeatable field link text should be the same (do you have a case where that's not true?).

Would definitely enjoy chatting some more about how we're using Kithe. For example, pulling our AttrJson attributes from a database table works well enough, but it also requires us to reload the application (and sidekiq) when our schema changes -- not ideal, but manageable, and not unlike say solr config changes.

@jrochkind
Copy link
Contributor

Yeah, I just think the field label and the repeatable field link text should be the same

That seems fine.

For example, pulling our AttrJson attributes from a database table works well enough, but it also requires us to reload the application

Yes. I am aware of "dynamic attributes" as a use case -- someone on attr_json github issues just expressed it too.

But honestly have not come up with any good solution for it -- I didn't realize you were doing it, and actually wouldn't have been confident it would have worked at all!

I'm not really sure how to achieve it with the design goals of "as much like ordinary ActiveRecord attributes as possible", since ordinary ActiveRecord attributes have a lot of assumptions baked in that they will be class level, and the way ruby works that makes it hard to change them on the fly dynamically. So I'm not totally sure if attr_json is really suitable... although am interested that you have made it work anyway!

But I'd love to hear more some time.

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

No branches or pull requests

2 participants