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

Disable "Add to cart" button for negative number of items #168

Closed
wants to merge 0 commits into from

Conversation

DurkoMatko
Copy link
Contributor

fix: add to cart button disabled for negative number of items

@HaGuesto HaGuesto requested review from jamescrowley and removed request for HaGuesto September 8, 2019 11:20
@jamescrowley
Copy link
Contributor

jamescrowley commented Sep 9, 2019

@DurkoMatko Thanks! I noticed in my testing using 'onchange' on the number field means that the 'Add to cart' button doesn't re-enable until I click outside of the number field, which may be confusing.

Did you happen to look at using the 'min' html attribute on the field instead of custom JS to enforce this?

@DurkoMatko
Copy link
Contributor Author

DurkoMatko commented Sep 9, 2019

@jamescrowley nope, I haven't. Will look at it today in the evening. The outside click is indeed needed, I found out yesterday while writing tests as well :D

Copy link
Contributor

@jamescrowley jamescrowley left a comment

Choose a reason for hiding this comment

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

👍 suspect if we can do the 'min' attribute, then none of the JS update is needed. but if it is, then yah will probably need to listen to 'click' in addition to 'onchange'.

@DurkoMatko
Copy link
Contributor Author

@jamescrowley hmm 'min' doesn't work...there's also try to use it in generating QR codes. That field also is supposed to have min value (see qr.php) but the form.php which is responsible for generating fields seems to ignore it..No idea why.

I could add some handler on the field which would automatically revert value to 1 if user input is less then or equal to zero? Or spend more time trying to make the 'min' happen..but probably during the weekend first.

Not sure why you suggest listening to click event.

@jamescrowley
Copy link
Contributor

@jamescrowley hmm 'min' doesn't work...there's also try to use it in generating QR codes. That field also is supposed to have min value (see qr.php) but the form.php which is responsible for generating fields seems to ignore it..No idea why.

Had a bit of a dig, pretty sure we need to add

  {if $element['min']}minlength="{$element['min']}"{/if}

to https://github.com/boxwise/dropapp/blob/master/templates/cms_form_number.tpl

Not sure why you suggest listening to click event.

Sorry, ignore - I remember some cases you had to listen to click in addition to on change when the focus left fields... suspect it would have to be something else in this case, but if ^^^ works we shouldn't need it

@DurkoMatko DurkoMatko closed this Sep 11, 2019
@DurkoMatko DurkoMatko force-pushed the fix-negative-number-of-items-in-checkout branch from 1156714 to f0bc16f Compare September 11, 2019 20:36
@DurkoMatko
Copy link
Contributor Author

DurkoMatko commented Sep 11, 2019

???
@jamescrowley No idea what happened here. I've rebased the branch to totally remove my initial commit with JS changes without leaving it in history. Then force-pushed. That automatically closed the PR. Then I've followed this article (https://evilmartians.com/chronicles/git-push---force-and-how-to-deal-with-it) to reopen the PR with my new changes...I've pushed my changes and they didn't appear anywhere here on the server...and now it says it got successfully merged? Is it saying it got merged cuz there's no difference between this branch and master (because I've rebase removed the only commit which was different?) No idea...I'll just push the 'min' change from new branch

Update: here new PR - #176

@jamescrowley
Copy link
Contributor

jamescrowley commented Sep 12, 2019

@jamescrowley No idea what happened here. I've rebased the branch to totally remove my initial commit with JS changes without leaving it in history. Then force-pushed. That automatically closed the PR.
Yeah, that happened because you ended up with no commits with changes after your re-base - not quite sure how that happened though.

Now that you got it working on a new branch, you could do something like

git checkout fix-negative-number-of-items-in-checkout
git reset add-min-attribute-to-number-input-template --hard
git push --force

which would check out this branch, reset it to the same as add-min-attribute-to-number-input-template, and push force here. But no need - I'll just review on the other PR

@jamescrowley jamescrowley deleted the fix-negative-number-of-items-in-checkout branch October 5, 2019 16:20
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.

2 participants