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.

Sign up for GitHub

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

Jump to bottom

⌨️Add autocomplete widget with command line completion #123

Merged
merged 4 commits into from
Aug 31, 2019
Merged

⌨️Add autocomplete widget with command line completion #123

merged 4 commits into from
Aug 31, 2019

Conversation

NBTX
Copy link
Contributor

@NBTX NBTX commented Aug 31, 2019 •
edited

Test URL: https://nbtx.github.io/postwoman-vue/

postwoman-autocomplete-demo

This PR:

@TravisBuddy
Copy link

Hey @NBTX,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 930f1880-cb90-11e9-81b8-6fab94e294e1

@NBTX NBTX requested a review from liyasthomas August 31, 2019 01:43
@TravisBuddy
Copy link

Hey @NBTX,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 4495b110-cb94-11e9-81b8-6fab94e294e1

@liyasthomas
Copy link
Member

Rebase and pull

liyasthomas
liyasthomas approved these changes Aug 31, 2019
View reviewed changes
Copy link
Member

@liyasthomas liyasthomas left a comment

Choose a reason for hiding this comment

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

🆗👌

@liyasthomas liyasthomas added the feature New feature or request label Aug 31, 2019
@TravisBuddy
Copy link

Hey @NBTX,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: bd756470-cc09-11e9-bfde-639d9c8dd8b4

liyasthomas
liyasthomas approved these changes Aug 31, 2019
View reviewed changes
Copy link
Member

@liyasthomas liyasthomas left a comment •
edited

Choose a reason for hiding this comment

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

Merging

@liyasthomas liyasthomas merged commit 7c49f58 into hoppscotch:master Aug 31, 2019
@liyasthomas liyasthomas mentioned this pull request Aug 31, 2019
Merged
@liyasthomas
Copy link
Member

liyasthomas commented Aug 31, 2019 •
edited

Had conflicts with binding query parameters with URL. Had to revert. Fix in your repo and pull again.

@NBTX
Copy link
Contributor Author

NBTX commented Sep 1, 2019

Crap, I did try it after I merged it. Do you know what went wrong?

@liyasthomas
Copy link
Member

liyasthomas commented Sep 1, 2019 •
edited

Screenshot_20190901-072705_Chrome
This is from your repo. Most prolly #118 caused this. This was the only commit before rebase 32218cf

@yubathom
Copy link
Contributor

yubathom commented Sep 1, 2019

Sorry about that. I can review it if you want..

@liyasthomas
Copy link
Member

Yeah sure. Please review.

@NBTX
Copy link
Contributor Author

NBTX commented Sep 2, 2019

Ah! I think I worked out what the issue was. I had a commit to stringify request data for POST, etc. but presumably you did a GET or a request without a body.
I'll just fix that and submit another PR.

@liyasthomas
Copy link
Member

It was GET 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liyasthomas liyasthomas liyasthomas approved these changes

Assignees
No one assigned
Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Content-Type revamping Add support for "application/hal+json" Content-Type
5 participants
@NBTX @TravisBuddy @liyasthomas @yubathom @SamJakob