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

Fix: terminate process on entering multiple slash #824

Merged
merged 6 commits into from
Jul 12, 2021

Conversation

ezkemboi
Copy link
Contributor

@ezkemboi ezkemboi commented Feb 1, 2021

Overview

Brief description of what this PR does, and why it is needed.

Demo

Optional. Screenshots, curl examples, etc.

Notes

Optional. Ancillary topics, caveats, alternative strategies that didn't work out, anything else.

Testing Instructions

  • How to test this PR
  • Prefer bulleted description
  • Start after checking out this branch
  • Include any setup required, such as bundling scripts, restarting services, etc.
  • Include test case, and expected output

@ezkemboi
Copy link
Contributor Author

ezkemboi commented Feb 1, 2021

Cc. @thewahome

@ddyett
Copy link

ddyett commented Feb 1, 2021

@thewahome can you describe the bad from the original issue. I'm less concerned that it fired and failed as expected but it seems that if you backspace we don't get suggestions again. Fixing that would be good.

Also note that this call in graph explorer does work
https://graph.microsoft.com/v1.0/me//?$select=displayname

@ezkemboi
Copy link
Contributor Author

ezkemboi commented Feb 1, 2021

@ddyett , I am going ahead to work on fixing that backspacing issue.

@ezkemboi
Copy link
Contributor Author

ezkemboi commented Feb 1, 2021

Removed double slash fix and added a fix for autosuggestion on backspacing.

Cc. @thewahome and @ddyett

@thewahome
Copy link
Collaborator

@ezkemboi the force push seems to have overwritten the previous slash changes...

@ezkemboi
Copy link
Contributor Author

ezkemboi commented Feb 2, 2021

@thewahome, I have added it back.

Comment on lines -191 to -197
let filteredSuggestions = parametersWithVerb.links;
if (compare) {
filteredSuggestions = filteredSuggestions.filter((suggestion: string) => {
return suggestion.toLowerCase().indexOf(compare.toLowerCase()) > -1;
});
}

this.setSuggestions(filteredSuggestions);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ezkemboi why was this removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Found that the branch was just behind the main repo's dev.

@thewahome thewahome self-requested a review July 12, 2021 12:20
@thewahome thewahome merged commit adbbc9f into microsoftgraph:dev Jul 12, 2021
thewahome added a commit that referenced this pull request Jul 15, 2021
* Fix: url truncation (#1012)

* Fix: improve themes MessageBar colors (#1015)

* Fix: Response Preview truncation (#1017)

* Feature: Have permissions be filtered (DelegatedWork or DelegatedPersonal) based on account type (#1008)

* Fix: terminate process on entering multiple slash (#824)

* Task: change sandbox url (#992)
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.

Entering multiple '/' triggers HTTP requests to the back end.
3 participants