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

Use DOM history API for navigating directories #3115

Merged
merged 4 commits into from Dec 13, 2017

Conversation

Projects
None yet
5 participants
@gnestor
Copy link
Contributor

gnestor commented Dec 7, 2017

Closes #307

@gnestor

This comment has been minimized.

Copy link
Contributor Author

gnestor commented Dec 7, 2017

This passes my basic user testing but I'm pretty sure there are some edge cases that may break this so it's marked as WIP.

@rgbkrk

This comment has been minimized.

Copy link
Member

rgbkrk commented Dec 8, 2017

Awesome thanks for going after this!

One edge case worth trying — do a right click and “open in new tab”

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Dec 8, 2017

This is great @gnestor! We'll be cribbing this for use with lab/tree. cf jupyterlab/jupyterlab#2532

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Dec 8, 2017

cc @afshin

@afshin

This comment has been minimized.

Copy link
Member

afshin commented Dec 9, 2017

Thanks for the heads up, @blink1073. Nice one, @gnestor!

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Dec 13, 2017

Nice, this feels super snappy. :-) I've tried opening in a new tab as @rgbkrk suggests, and it seems to work correctly.

However, something is going wrong with the breadcrumbs - when I click on them to navigate back up, the URL bar does not update. And if I navigate down three directories and then use the breadcrumbs to go up one, it requests the wrong thing (e.g. if I navigate to Code/astcheck/dist, and then click astcheck in the breadcrumbs, it requests astcheck instead of Code/astcheck).

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Dec 13, 2017

I pushed a couple of commits to fix things for me and make sure we're using base_url where necessary.

The test failures appear to be unrelated - #3127

@takluyver takluyver closed this Dec 13, 2017

@takluyver takluyver reopened this Dec 13, 2017

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Dec 13, 2017

@gnestor is there more you want to do with this, or shall we merge it so that people can try it out in master?

@gnestor

This comment has been minimized.

Copy link
Contributor Author

gnestor commented Dec 13, 2017

@takluyver Thanks for fixing the URL construction bits 👍 Let's merge!

@gnestor gnestor added this to the 5.4 milestone Dec 13, 2017

@gnestor gnestor merged commit 9ce534c into jupyter:master Dec 13, 2017

4 checks passed

codecov/patch Coverage not affected when comparing ae011a1...0061144
Details
codecov/project 78.52% remains the same compared to ae011a1
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gnestor gnestor deleted the gnestor:issue-307 branch Dec 13, 2017

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Dec 13, 2017

:-) As this is a user-visible change, it would be good to write a little bit about it for the release notes.

@gnestor

This comment has been minimized.

Copy link
Contributor Author

gnestor commented Dec 13, 2017

Is this the same test error that you saw before: https://travis-ci.org/jupyter/notebook/builds/316101183?utm_source=email&utm_medium=notification

Ok, I'll update release notes now...

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Dec 14, 2017

No, that's a different failure. It seems to be that the linkcheck is failing because nbviewer is down.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Dec 14, 2017

@gnestor gnestor changed the title [WIP] Use DOM history API for navigating directories Use DOM history API for navigating directories Jan 5, 2018

@gnestor gnestor referenced this pull request Jan 5, 2018

Closed

Release 5.3 #3183

9 of 9 tasks complete

@takluyver takluyver modified the milestones: 5.4, 5.3 Jan 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.