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

Switch to webview api #174

Closed
wants to merge 10 commits into from
Closed

Switch to webview api #174

wants to merge 10 commits into from

Conversation

abe33
Copy link
Contributor

@abe33 abe33 commented Nov 8, 2018

Ref #172

  • Status panel
  • Install flow
  • Fixing Install flow styles (@dhung09 maybe there's a pass to do here to make it consistent with atom)

@abe33 abe33 force-pushed the cn-switch-to-webview-api branch 2 times, most recently from 40a3529 to 8cbe5d9 Compare November 14, 2018 15:13
@@ -4,8 +4,7 @@ const os = require('os');
const vscode = require('vscode');
const {workspace} = vscode;
const formidable = require('formidable');
Copy link
Contributor

Choose a reason for hiding this comment

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

unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch, indeed we no longer need that as it was used to parse form data sent by the page when we had to rely on a server started by the extension.

Copy link
Contributor

@dbratz1177 dbratz1177 left a comment

Choose a reason for hiding this comment

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

Code looks good and makes sense. My comments are more UX related (cc @dhung09 ):

  • Status Panel:
    • I like the responsiveness with changes in login state
    • Should Kite Search go to the web docs (where it goes currently) or the copilot? All the other links go to the copilot. If we decide to keep it pointing to the webdocs, perhaps we should label it differently (e.g. Kite Web Docs)
    • We should not show Permissions in Kite Local, if possible
    • When I hover on the links in the Status Panel, I get popups with what, from a user perspective, appears to be gobbledegook. I'm guessing we should probably get rid of that
    • There was no apparent action taken when I clicked Invite Friends
  • Install Panel (with no Kite installed)
    • When I enter a new email, no apparent action takes place. I do get a verification email. If I verify that email, then take no further action (I'm asked to create a password), and then restart the VSCode install flow, I get to a point where I seem to have a passwordless account, and can't move forward from that point
      • is there a reason a verification email is being sent here? Shouldn't I be able to just create an account without that extra step?
    • If I enter an extant email, then I can continue when I click Continue. However, no apparent action is taken when I enter the correct password
    • When I click "Forgot Password", no apparent action is taken, though a reset password email is sent
    • Because of being unable to get past a couple of the steps above, I wasn't able to fully evaluate the rest of the flow

@abe33
Copy link
Contributor Author

abe33 commented Nov 20, 2018

I've just made a pass on the status view, the install flow will come after

Should Kite Search go to the web docs (where it goes currently) or the copilot? All the other links go to the copilot. If we decide to keep it pointing to the webdocs, perhaps we should label it differently (e.g. Kite Web Docs)

I do agree this is definitely weird. Whatever solution is fine with me though. And even if we continue to point to the webdocs, do we still need the desktopLogin thing? (this is also true for Atom btw)

We should not show Permissions in Kite Local, if possible

I guess this is a change we'll be able to address with kite-api 2.2 and your isLocal function.

When I hover on the links in the Status Panel, I get popups with what, from a user perspective, appears to be gobbledegook. I'm guessing we should probably get rid of that

Oh yeah I didn't noticed that mostly because I was clicking on them rather that hovering. Apparently it happens because we don't provide any title on the link. The solution here is to define that attribute for all the links, I'll make the change and we can change the wording later if needed.

There was no apparent action taken when I clicked Invite Friends

That was indeed broken, I missed these links when updating to postMessage. Now, these links were pointing to http://localhost:46624/redirect/invite and I get an error on https://kite.com/invite after the redirection. Is that still a thing?

@abe33
Copy link
Contributor Author

abe33 commented Nov 20, 2018

@dbratz1177 For the install flow, when you talk about no action being done, are you talking about visual feedback? That was some of the things I was considering as improvements.
As for the install not doing anything after providing the credentials I'm not sure what could happen, any error should move the flow to the error step (unless it's explicitly handled by the step, maybe that's what is broken) and in case we successfully authenticate on kite.com we start the download+install on a new view (the same install wait view that Daniel added for atom). I'll look into this.

@dbratz1177
Copy link
Contributor

dbratz1177 commented Nov 20, 2018

That was indeed broken, I missed these links when updating to postMessage. Now, these links were pointing to http://localhost:46624/redirect/invite and I get an error on https://kite.com/invite after the redirection. Is that still a thing?

It's working for me (both the https://kite.com/invite and the redirection). And I think it's still a thing, though I do know we're going to be phasing out our current Pro iteration

For the install flow, when you talk about no action being done, are you talking about visual feedback?

After a second look, this is probably the essential issue that I was having... if we're requiring users to take action outside of VSCode to complete the installation process, I think we have to provide them some sort of feedback as to what to do next, and what to expect from the rest of the process.

@abe33
Copy link
Contributor Author

abe33 commented Nov 21, 2018

I retried the scenario of installing with a newly created account and yes the install do start after a while, we can definitely do better here though. It looks to me that when we submit the form with the new email we should move to a new screen explaining what's going to happen.
From what I could see when trying I think the email verification and then account creation is taking an unexpected amount of time to perform only two requests to kite.com (one to /api/account/check-email and the other to /api/account/createPasswordless). When we get the response from the /api/account/createPasswordless endpoint we directly jump to the download phase and in that case we change the view. Is there any issue on kite.com side that could explain that long delay ? (I think initially we never bother add a intermediate view given the two request should be done super quickly).

@dbratz1177
Copy link
Contributor

@abe33 I'm not aware of any issue with the backend that's generating the slowness.
the solution of having an explanatory intermediary screen seems good to me too

@dbratz1177
Copy link
Contributor

@abe33 another option to an intermediary screen would be to just add a spinner like the installation page

@abe33
Copy link
Contributor Author

abe33 commented Dec 12, 2018

@dbratz1177 @dhung09 Added two new views for email verification and account creation, it seems it's the account creation that takes an extra time to end. Feel free to modify the views content.

Copy link
Contributor

@dbratz1177 dbratz1177 left a comment

Choose a reason for hiding this comment

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

Changes look good to me - my concerns were largely addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants