Update backend New Image link for consistency

[brchristian]

This makes the New Image link consistent with visual style of other backend views.

Other Views:

Current Behavior:

New Behavior:

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)
• I have attached screenshots to this PR for visual changes (if needed)

[aldesantis]

Thanks @brchristian!

[jarednorman]

Looks like you've applied the new_image_link ID to both the li and the a elements. Since IDs should be unique and it's conceivable that someone could be using deface overrides to target this, I think it's best to leave the ID on the a and not also add it to the li.

[brchristian]

@jarednorman Good catch!

Looking around at other backend views, it seems like in https://github.com/solidusio/solidus/blob/master/backend/app/views/spree/admin/prices/index.html.erb#L5-L7 and https://github.com/solidusio/solidus/blob/master/backend/app/views/spree/admin/variants/index.html.erb#L7-L13, the convention is to put the ID on the li element. I’d suggest that we do the same here for consistency with those other views.

I’ve force-pushed the latest; LMK what you think.

[kennyadsl]

Thanks @brchristian !

[brchristian]

Ahh, actually there is more to the story here, because there are both specs and JS that expect the ID to live on the a element:

solidus/backend/spec/features/admin/products/edit/images_spec.rb

Line 43 in 10a1199

click_link "new_image_link"

solidus/backend/app/assets/javascripts/spree/backend/images/index.js

Line 2 in 10a1199

$('#new_image_link').click(function(event) {

For now, I have reversed my earlier decision and have force-pushed a version that puts the ID on the a and not the li. This is inconsistent with admin/prices.index and admin/variants/index, which use the li:

solidus/backend/app/views/spree/admin/prices/index.html.erb

Lines 5 to 7 in 10a1199

	<li id="new_price_link">
	<%= link_to t(".new_price"), new_object_url, class: 'btn btn-primary' %>
	</li>

solidus/backend/app/views/spree/admin/variants/index.html.erb

Lines 7 to 13 in 10a1199

	<li id="new_var_link" data-hook>
	<%= link_to(
	t('spree.new_variant'),
	new_admin_product_variant_url(@product),
	class: 'btn btn-primary'
	) %>
	</li>

...so my OCD is triggered, but on the other hand, putting the ID on the a element: (1) doesn't change from the current behavior, which already has the ID on the a element, and (2) doesn't require changing the specs or JS.

Probably a more principled solution would be to move the "new_image_link" ID to the li for consistency with other backend views, but to then come up with some other unique ID to put on the a element and have that ID in the spec and JS. However I'm not exactly sure what that other ID should be, because "new image link" is such an obvious name. :)

[aldesantis]

Thanks @brchristian! This triggered my OCD too... I guess another solution would be to update both the JS and the spec to just take the link that is contained in the li element (#new_image_link > a), but it's definitely not a priority. 🙂