Resolve relative URLs in markdown cells to notebook directory #1432
Conversation
| const { node } = event; | ||
| if (event.entering && node.type === 'link') { | ||
| node.destination = url.resolve(cwd, node.destination); | ||
| } |
| @@ -123,6 +118,23 @@ export default class MarkdownCell extends React.PureComponent { | |||
| } | |||
|
|
|||
| render(): ?React.Element<any> { | |||
| const { filename } = this.context.store.getState().metadata.toJS(); | |||
There was a problem hiding this comment.
We shouldn't be relying on this.context in these components. Perhaps we could pass down what the cwd is?
There was a problem hiding this comment.
Ya, I couldn't figure out how to access it otherwise. Your <base> idea sounds better if I understand it correctly: it's a component that has access to cwd among other things and it passes it down to other components via props.
There was a problem hiding this comment.
<base> is an HTML Element that specifies the base URL to use for all relative URLs in a document, it's not a React component. With the power of React though, when the filename changes, so would base's href and thereby the interpretation of all relative paths.
There was a problem hiding this comment.
Ohhhh! Ya, that's perfect except that it messes up relative URLs to nteract assets 🤔
There was a problem hiding this comment.
Yeah I was thinking that since we're doing webpack anyways we could make sure the base assets are loaded first. The ones that are gunked up in the little prototype I showed was only the theme css (which can change).
There was a problem hiding this comment.
I just pushed another commit that breaks the context stuff out into a Base component. I did this yesterday before I realized that <base> is an HTML element, so for what it's worth...
There was a problem hiding this comment.
You had me a bit worried when I saw the commit before your comment.
The component you've added Base into is supposed to be a disconnected component, in line with #1288.
|
Great to see this solution! This component is supposed to be a presentational component and as such should not have access to context - props are intended to be the only API in accordance with #1288. I'd like to solve this either with the The other issue with relying on context for this is that it will not necessarily re-render when the filename changes as opposed to when its a prop. |
I get that, but why does MarkdownCell and others define
This is ideal. |
The cell components are currently open check boxes on that issue. I'd rather not introduce more to refactor when we have other options for doing this in line with the architecture plan. |
|
Ok I see. Props-only approach is vastly superior 👍 |
|
From direct messages, I'd love to see this done with |
|
Closing in favor of #1720. |
|
This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread. |
Follow up from #349
To do:
<base>html element: Handle ipynb local links directly #349 (comment)