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/prev next links issue #10523

Merged
merged 16 commits into from
Jun 13, 2020
Merged

Fix/prev next links issue #10523

merged 16 commits into from
Jun 13, 2020

Conversation

MaddyDev
Copy link
Contributor

@MaddyDev MaddyDev commented May 20, 2020

Addressing #10501
Previous and next navigation buttons are rendered correctly only on one viewlet at any point of time, depending on which one has the last book opened. That's because every time we open a book, we're registering a new navigationProvider with the same providerid which is basically overriding the existing ones.

In this PR I added a different provider for each viewlet and while retrieving the correct provider used the existing context key to get the appropriate one.

Am not sure if there are better ways to do this, so would like your feedback for improving this.

@coveralls
Copy link

coveralls commented May 20, 2020

Coverage Status

Coverage increased (+0.04%) to 34.611% when pulling 25fe566 on fix/prevNextLinksIssue into a7110d8 on master.

@MaddyDev MaddyDev requested a review from chlafreniere June 4, 2020 16:55
Copy link
Contributor

@Charles-Gagnon Charles-Gagnon left a comment

Choose a reason for hiding this comment

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

A few smaller suggestions but overall seems fine - will need to run it through some testing though to be sure

@@ -186,7 +186,7 @@
}
},
{
"command": "notebook.command.searchUntitledBook",
"command": "notebook.command.searchProvidedBook",
Copy link
Contributor

Choose a reason for hiding this comment

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

are there any places where we're referencing notebook.command.searchUntitledBook?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we use it for search on the items in Provided Books viewlet.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. just wanted to make sure that anything that referenced the old command name has been updated.

Copy link
Contributor

@chlafreniere chlafreniere left a comment

Choose a reason for hiding this comment

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

Just a couple of minor things, but overall looks good to me. Thanks for getting this in.

Please test this thoroughly to make sure that we're not regressing anything here.

@@ -120,9 +118,9 @@ export async function activate(extensionContext: vscode.ExtensionContext): Promi
}

let workspaceFolders = vscode.workspace.workspaceFolders?.slice() ?? [];
const bookTreeViewProvider = new BookTreeViewProvider(appContext.apiWrapper, workspaceFolders, extensionContext, false, BOOKS_VIEWID);
const bookTreeViewProvider = new BookTreeViewProvider(appContext.apiWrapper, workspaceFolders, extensionContext, false, BOOKS_VIEWID, NavigationProviders.NotebooksNavigator);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is totally random but this reminds me of a baseball team near where i grew up:

Norwich naviagators

@MaddyDev MaddyDev merged commit 26a0069 into master Jun 13, 2020
@MaddyDev MaddyDev deleted the fix/prevNextLinksIssue branch June 13, 2020 01:42
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.

5 participants