NEW update button added to checkout process #33
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fspringveldt
force-pushed
the
issue/11
branch
2 times, most recently
from
521a566
to
da04a88
Compare
|
Codecov dropped. I'll address that pronto. |
fspringveldt
force-pushed
the
issue/11
branch
3 times, most recently
from
847235b
to
e9cf1b5
Compare
sachajudd
suggested changes
Jun 1, 2017
css/dms-cart.css
Outdated
| } | ||
|
|
||
| .checkout-page-thumbnail .thumbnail { | ||
| width: 32px; | ||
| border: none; | ||
| vertical-align: middle; | ||
| } | ||
|
|
||
| .Actions a.action:hover{ |
There was a problem hiding this comment.
Spaces, please follow above! (Also for line 12)
robbieaverill
suggested changes
Jun 1, 2017
code/classes/DMSDocumentCart.php
Outdated
| */ | ||
| public function setViewOnly($updating) | ||
| { | ||
| $this->viewOnly = (bool) $updating; |
There was a problem hiding this comment.
might make more sense to call this $viewOnly
code/classes/DMSDocumentCart.php
Outdated
| ) { | ||
| return Controller::join_links($url, $action); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you rubber duck the logic this method to me?
| @@ -166,7 +167,7 @@ public function testCannotAddMoreThanSuggestedQuantityOfItem() | |||
| { | |||
| $document = $this->objFromFixture('DMSDocument', 'limited_supply'); | |||
| $result = $this->get('/documentcart/add/' . $document->ID . '?quantity=5&ajax=1'); | |||
| $this->assertContains('You can\'t add 5 of this document', (string) $result->getBody()); | |||
| $this->assertContains('You can\'t add 5 of', (string) $result->getBody()); | |||
There was a problem hiding this comment.
Maybe this assertion could also cover the new "title" in the message too
| public function testView() | ||
| { | ||
| $result = $this->get('documentcart/view'); | ||
| $this->assertInstanceOf('SS_HTTPResponse', $result); |
There was a problem hiding this comment.
this assertion seems a bit pointless TBH
robbieaverill
approved these changes
Jun 1, 2017
sachajudd
approved these changes
Jun 1, 2017
There was a problem hiding this comment.
A few minor UX issues but these will be addressed in a later commit. Nice @fspringveldt will merge after tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduced a new summary method to cart which displays it's content either in edit/view mode depending on Cart::$update setting.
This change caused issue #32 and was raised accordingly.
Fixes #11