replace link_to_add_fields usage and deprecate helper function

[hefan]

Description

Problem:
The link_to_add_fields function will (when used for buttons) create the following markup
<a ...> <span class="text">Add Option Value</span> </a>
The span with class text will be removed when the menu is minimized.
So we only see the plus icon with no text when the menu is minimized.

This happens in the option type edit screen and in the product properties index screen.
/admin/option_types/1/edit
/admin/products/ruby-hoodie/product_properties (2 occurrences)

See Screenshot:

Solution:
This PR removes the symbols from the buttons just by using link_to instead of link_to_add_fields.
The Buttons will have no more symbol but the text stays when the menu is minimized.
These three buttons where the only buttons with symbols in the Backend.

The helper function is also deprecated

See also #3544

[kennyadsl]

Thanks, I left some comments on the implementation, let me know your thoughts.

[hefan]

i guess the specs also need a rewrite, click_button instead of click link. will do that later.

[hefan]

ok. all works fine now

[kennyadsl]

Just a comment on the erb indentation, the rest is 👍

[kennyadsl]

Thanks!

[spaghetticode]

@hefan thanks for working on this! I was thinking, what about adding a new helper method? It would allow to DRY-up a bit your solution, as very similar ERB code is repeated 3 times.

[kennyadsl]

@spaghetticode I'm 👎for creating a new helper. We can change a use the one we already have (removing the deprecation) but I personally think we should move to less custom helpers and more what's already provided by Rails, at least in backend views.

[spaghetticode]

I personally think we should move to less custom helpers and more what's already provided by Rails, at least in backend views

@kennyadsl I would like to better understand your opinion, can you explain to me more in detail the reason for this sentence?

[hefan]

Aren't there many helper functions in base_helper and navigation_helper for creating Buttons or Links which couldn't be done also with the normal Rails helper Functions? i found them confusing and they seem to do nearly the same thing. Also they do things no longer needed (the icons on buttons for example seem to be removed).

[kennyadsl]

I don't think a wrapper that only adds a couple of attributes to an existing Rails helper is worth keeping, here's why:

• abstraction: from the perspective of who is understanding how that part works, there are no notable differences between using a helper method or adding a class to an element. It's just one more step.
• deprecations: yes, having a helper could be helpful to print deprecation warnings but no one ensures us that stores are not using the JS part of this functionality only, bypassing the provided helpers already. This will end up having us deprecate two things instead of one.

Personally, and for consistency, I'd deprecate link_to_remove_fields as well. Maybe in another PR?

[spaghetticode]

@kennyadsl when a store needs to change the behavior, a helper would be helpful in not making necessary to change any views (the occurences are 3 in 2 view files this time). Decorating the helper would do... that's the main benefit I see about using a helper here. Then there is reusability, when one wants to add a similar button somewhere else.

Three instances is the bare minimum number of occurrences to consider DRYing up the code, so I don't see any problem in postponing this later if it will ever make more sense. Also, taking in account how simple is the code (it's a bit verbose on 3 lines, but no if clause is present for example), repeating makes probably more sense.

[spaghetticode]

@hefan thank you 👍

[hefan]

So i would do a PR removing and deprecating link_to_remove_fields, ok? there is only one occurence.
About the 3 Lines: Basically the new method produces not much more code than using the old helper method. i found it more readable in 3 lines. i am not sure about the best practice in erb with the long param list. Should i keep it in seperate lines or should i prefer longer lines?

[kennyadsl]

@hefan if you have time, a PR to remove link_to_remove_fields as well would be appreciated, thanks again!