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

Fix duplicated Shipments breadcrumb #1717

Merged

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Feb 17, 2017

🤣 This is a silly one

We had the admin_breadcrumb declaration too deep within our partials, so it was called once per shipment, rather than a single time.

This moves the call so it is added only once.

Before

After

@@ -1,3 +1,5 @@
<% admin_breadcrumb(plural_resource_name(Spree::Shipment)) %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to always have admin_breadcrumb at the top of every non-partial template?

Like this we ensure that the method is called only once. Otherwise, we can't reuse _form partial without getting the "Shipments" breadcrumb, no matter which page we're on.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vfonic has a valid point. I think we should move it into the edit template.

@tvdeyen
Copy link
Member

tvdeyen commented Feb 17, 2017

Just for the record: We should start to think about how we organize the admin order templates and partials. Ideally we have a template for each order state (or better tab). Having an edit template for shipment state is somehow silly.

@jhawthorn
Copy link
Contributor Author

jhawthorn commented Feb 17, 2017

Just for the record: We should start to think about how we organize the admin order templates and partials. Ideally we have a template for each order state (or better tab). Having an edit template for shipment state is somehow silly.

The names aren't ideal, but it's never made visible to the user. IMO it's not worth changing them due to the incompatibilities it will cause. Either way, outside the scope of this PR.

@jhawthorn jhawthorn force-pushed the duplicated_shipments_breadcrumb branch from bdea5f4 to 6efe21b Compare February 17, 2017 21:00
@vfonic
Copy link
Contributor

vfonic commented Feb 18, 2017

The names aren't ideal, but it's never made visible to the user. IMO it's not worth changing them due to the incompatibilities it will cause. Either way, outside the scope of this PR.

I think that developers should also be considered users. It will be easier for solidus developers to jump on certain code part and make changes, as well as for solidus gem users, to figure out how to use the solidus code API (not JSON API).

@jhawthorn jhawthorn dismissed tvdeyen’s stale review February 22, 2017 01:13

This has been addressed.

@jhawthorn jhawthorn merged commit 5d77624 into solidusio:master Feb 22, 2017
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

Successfully merging this pull request may close these issues.

4 participants