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

ES6+ Syntax & API Separation #1

Closed
wants to merge 65 commits into from
Closed

ES6+ Syntax & API Separation #1

wants to merge 65 commits into from

Conversation

walmat
Copy link
Owner

@walmat walmat commented Nov 16, 2019

No description provided.

@walmat
Copy link
Owner Author

walmat commented Nov 19, 2019

Manual Checks to test:

  • Can the user successfully login
  • Can the user place an ask
  • Can a user update that previously placed ask
  • Can a user place a bid
  • Can a user update that previously placed bid
  • ..anything else?

@walmat
Copy link
Owner Author

walmat commented Nov 19, 2019

@matthew1232 let me know what is left to test, I'll run through a code review of this right now in it's current state.

Copy link
Owner Author

@walmat walmat left a comment

Choose a reason for hiding this comment

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

First pass done.

examples/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Collaborator

@click2install click2install left a comment

Choose a reason for hiding this comment

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

Some general observations. Not sure at what stage the PR is at, most comments are w.r.t the code being made more DRY and a few small other observations.

src/apis/stockx/asks/index.js Outdated Show resolved Hide resolved
src/apis/stockx/asks/index.js Outdated Show resolved Hide resolved
src/apis/stockx/asks/index.js Outdated Show resolved Hide resolved
src/apis/stockx/index.js Outdated Show resolved Hide resolved
src/apis/stockx/user/helpers.js Show resolved Hide resolved
src/apis/stockx/index.js Outdated Show resolved Hide resolved
matthew1232 and others added 22 commits November 20, 2019 14:02
Login credential status/general request errors
Implementing error handling from utils and changed checkStatus to submitCallback
Removed unnecessary error bubbling
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Include 304 as accepted status code
Import error handling into utils
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Updated error handling, removed unnecessary error bubbling, adding re…
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Copy link
Owner Author

@walmat walmat left a comment

Choose a reason for hiding this comment

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

Opened remaining separate issues and resolved. Should re-validate manual checks before merging!

matthew1232 and others added 6 commits November 24, 2019 20:39
Co-Authored-By: Matthew Wall <matthew.wallt@gmail.com>
Added deleting bids, implemented checking status from utils
Allowed objects from products.search, fixed error handling and removed error bubbling that was unneeded.
Add error handling from utils
Remove renamed variables (fatal)
@walmat walmat closed this Aug 27, 2024
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.

3 participants