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

Anchorage #244

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
4 participants
@mpacer
Copy link
Member

commented Nov 4, 2018

This is a PR that should make it easier to navigate to the cell that was in error when you run into an error. Specifically, it allows you to click on a link that jumps you right to the cell where the error occurred.

How does it do this?

  1. Give each cell a unique id
  2. Create a new collection of cells that add anchors to the page tying that unique id to that location in the DOM
  3. Modifies the error message being inserted at the top of the page to include an anchor link to the cell in error.

It does nothing if there is no error.

I think I'd want to include tests for the new apis that this introduces before we released this, but I want to iterate a couple of times first on the exact form this should take.

But nonetheless the existing tests needed to be modified to pass as of this latest commit.

mpacer added some commits Nov 4, 2018

@mpacer mpacer force-pushed the mpacer:anchorage branch from 737afba to 0fd0805 Nov 4, 2018

@mpacer

This comment has been minimized.

Copy link
Member Author

commented Nov 4, 2018

@MSeal this is sort of what I was thinking of when I suggested making the cell reference in the error message cell to the cell in question.

@codecov

This comment has been minimized.

Copy link

commented Nov 4, 2018

Codecov Report

Merging #244 into master will increase coverage by 9.94%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage   80.69%   90.64%   +9.94%     
==========================================
  Files          13       14       +1     
  Lines        1233     1101     -132     
==========================================
+ Hits          995      998       +3     
+ Misses        238      103     -135
@mpacer

This comment has been minimized.

Copy link
Member Author

commented Nov 4, 2018

Here is a quick gif showing the basic result.

clickable_cell_id_links

@MSeal

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

This is a neat idea. I like the functionality but I am a little hesitant to add all the extra cells. From a presentation aspect it looks mostly right (maybe hiding the cell ID output as well from display) but from a model pov we have many extra cells. Classic notebook would render this oddly today and the cell execution counts would (I believe?) all be even numbers with the markdown cells in-between.

Do you think maybe we could insert a hidden html output on each cell which includes the cell id in the same manner as this? That's eliminate the cell count / visualization issue but let you stlll jump to failure outputs?

I'll see if I can chat 1:1 with you to see if we can iterate on the approach and desired end-point offline

@willingc

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

I like the concept of this especially if the notebook has 20+ cells. Is there a way to suppress the output of the Cell IDs?

@rgbkrk

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

I'd be happy if the cellID was in the top level metadata and that we could use the same entry when writing notebooks with nteract. We could then acknowledge this formally by providing ids to the cell div when we render them with commuter.

@mpacer

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

Updated this to use an anchor on the page as a target id, so that there's no text visible.
2018-11-29_10-17-02s

@willingc

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

This looks pretty cool in the latest GIF. @rgbkrk @MSeal thoughts?

@MSeal

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

The user interface in commuter looks good now. I do still worry about the logic of doubling the number of cells, especially with not all interfaces respecting hidden fields yet.

Iterating on the cells for an output notebook now would be quite different than iterating on the input cells. Would it be possible for us to add the display output to the existing cells rather than make new cells? Each cell could have a hidden html output linking to the next cell as it's final output. Thoughts on taking that approach?

@MSeal

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

Closing for now. We can return to this topic again at a later point, or maybe try to get something more standardized elsewhere for generic support of this capability..

@MSeal MSeal closed this Jun 29, 2019

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.