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

Improve anchor tags #4692

Merged
merged 5 commits into from Jul 11, 2018
Merged

Improve anchor tags #4692

merged 5 commits into from Jul 11, 2018

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 7, 2018

Supersedes #1945.

This fixes a longstanding issue with relative links between documents that involve a header anchor tag. Does the following:

  1. Our rendered markdown automatically generates header ids for markdown headers.
  2. The link handler for the rendermime-extension now opens the relative document, and then calls a node.querySelector to search for an element with the corresponding id. If it finds that element, we scroll it into view.
  3. Introduces a concept of defaultRendered for document widget factories, distinct from the default file type for that factory. This way we can have a file type (such as markdown), that opens with the text editor by default, but can also be opened with a defaultRendered factory. By default the linkHandler opens files with the defaultRendered.
@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 7, 2018

I'd like to get this in for Beta 3, since it involves a minor version bump to rendermime-interfaces.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 8, 2018

Taking a step back: Perhaps we should name the concepts default editor and default viewer? Should we insist that the default viewer be readonly? Right now I think we put the default editor as the first item on the open with list - should the default viewer be right under it? Right above it?

When double-clicking on a file, should we open it with the default editor or the default viewer? Should each filetype have a default default (i.e., should I use the default editor or the default viewer)?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 8, 2018

I don't think we really have a concept of a read-only viewer right now, but we should (see the discussion in #3915 and #3251). We have a read-only model, but if the model is writable, then so is the viewer (leading to odd things like being able to "save" in the image viewer).

Right now the behavior of the default widget factory is that it always returns a widget factory. In this PR, I have made it so that the defaultRendered function returns the default rendered factory if it is available, and then falls back on the default factory if it is not. If we were to put the default editor and default viewer on equal footing we would want to rethink that logic.

I agree about putting the default viewer right under the default editor in the Open With submenu.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 8, 2018

This discussion about making a distinction between "viewers" and "editors" is significantly larger than the particular issue that this PR is trying to address. What do you think the best way forward on that is?

@@ -303,6 +303,9 @@ abstract class ABCWidgetFactory<T extends IDocumentWidget, U extends DocumentReg
this._name = options.name;
this._readOnly = options.readOnly === undefined ? false : options.readOnly;
this._defaultFor = options.defaultFor ? options.defaultFor.slice() : [];
this._defaultRendered = options.defaultRendered ?
options.defaultRendered.slice() :
[];
Copy link
Member

@afshin afshin Jun 13, 2018

Choose a reason for hiding this comment

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

If you want to be cute:

this._defaultRendered = (options.defaultRendered || []).slice();

@jasongrout jasongrout removed this from the Beta 3 milestone Jun 27, 2018
@jasongrout jasongrout added this to the Beta 4 milestone Jun 27, 2018
* Get the default widget factory for an extension.
* Get the default rendered widget factory for a path.
*
* @param path - The path to for which to find a widget factory.
Copy link
Member

@blink1073 blink1073 Jul 3, 2018

Choose a reason for hiding this comment

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

the path for which?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 3, 2018

FWIW I like the name "rendered". View implies read-only to me. Also 👍 on listing this after the default entry in the Open with menu.

@ian-r-rose ian-r-rose force-pushed the improve_anchor_tags branch from aac5050 to d8b8677 Jul 11, 2018
@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jul 11, 2018

Rebased

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 11, 2018

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jul 11, 2018

Ah, right. Do you think it's worthwhile to run prettier in the jlpm run integrity?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 11, 2018

I think issue is that some things are not fixed by prettier. Does prettier not run for you locally on git commits?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jul 11, 2018

It does, but it was not picked up by my rebase.

I was more thinking: if the integrity test is checking prettier, then semantically I might expect that jlpm run integrity might apply it.

Prettier.
@ian-r-rose ian-r-rose force-pushed the improve_anchor_tags branch from d8b8677 to 88d6093 Jul 11, 2018
@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 11, 2018

Only if it is limited by git status: I wouldn't want to wait while it runs prettier on everything.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jul 11, 2018

Fair enough.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 11, 2018

Were you planning to update the Open With... menu in this PR as well?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jul 11, 2018

Yeah, I'll take a look at that today.

preferredWidgetFactories (after the default widget factory).
Copy link
Member

@blink1073 blink1073 left a comment

Thanks!

@blink1073 blink1073 merged commit b354907 into jupyterlab:master Jul 11, 2018
2 checks passed
@jasongrout jasongrout removed this from the 0.34 milestone Jul 19, 2018
@jasongrout jasongrout added this to the 0.33 milestone Jul 19, 2018
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants