Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

1 3 stable #22

Merged
merged 3 commits into from May 8, 2013

Conversation

Projects
None yet
7 participants
Contributor

binaryphile commented Jan 30, 2013

This is a candidate pull for a new master for spree_print_invoice, update for Spree 1.3.

I did this before I saw the other pull request to make this work with 1.3. I believe my pull request is somewhat better implemented, so I'm submitting it for your consideration.

Differences:

  • The other request loses 1.2 out of the Versionfile
  • This request specifies a new 1-3-stable branch as the branch for 1.3, not master. The pull should go into master and then be branched into 1-3-stable.
    • Master is unstable by definition and all versions should have a stable branch which is the suggested one in the Versionfile, just as all prior versions have.
  • This request adds the print invoice button in the toolbar, where all of the other action buttons go. This not only matches the layout in 1.2, it's also good UX to keep all action buttons together and consistent.

Aside from that, the requests are pretty much the same.

There are two basic changes. Since in 1.3 the admin buttons have been changed to an unordered list, the spree_print_invoice partial that adds the invoice button(s) has been updated to create list items rather than bare links.

The second change is that there were two overrides for 1.2, one for showing and one for editing. Because the toolbar buttons have been moved into a single view in 1.3, the override has also been consolidated into a single file. The override has been named to match the new view. Since the code is also executed on any admin page, not just orders, a call to test whether the current page is an order has been added. The button(s) will not be shown on any page that is not an order.

jerser commented Feb 7, 2013

Yours is indeed better, I closed mine.

Contributor

binaryphile commented Feb 7, 2013

Thanks, not trying to prove a point or anything, just trying to make it easy on the maintainers. :)

Contributor

linrock commented Mar 15, 2013

+1

erwin16 commented Apr 10, 2013

+1

JDutil added a commit that referenced this pull request May 8, 2013

@JDutil JDutil merged commit c81ae74 into spree-contrib:master May 8, 2013

Owner

JDutil commented May 8, 2013

Thank you.

I was using the master branch with my spree 1.2.4 project and now i cant bundle update without getting an error. Can we get 1.2 stable for people still using spree 1.2.x? I don't see one in the branch list.

Owner

JDutil replied Jun 1, 2013

Ive updated the versionfile to correctly list where to point your gemfile.

You should be able to update to:

gem 'spree_print_invoice', github: 'github/spree_print_invoice', ref: '2e52c3b79c3ae0e5fb5f502d0ec3dad46c82ad42'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment