Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

Internal link (from markdown) are normal link and trigger full page load #67

Closed
MoOx opened this issue Jan 6, 2016 · 20 comments
Closed

Comments

@MoOx
Copy link
Owner

MoOx commented Jan 6, 2016

That's a shame isn't it?
Before handling this, we need to work on #11 (cause this will handle markdown as react components instead of html).

@thangngoc89
Copy link
Contributor

I think this can be solve seperately with #11

I'm working on a branch which use markdown-it-react-renderer to parse markdown from rawBody and replace <a> tag with <Link> component from react-router

https://github.com/MoOx/statinamic/compare/babel-node-loaders...thangngoc89:use-renderer?expand=1

@thangngoc89
Copy link
Contributor

Maintaining a markdown it renderer is a big deal and a time-consuming task.

I chosed a different approach by using html-to-react to parse html data from markdown-it and replace <a> and <Link> component. This is easier and faster.

1 down side is they are using lodash without cherry picking, so this will increase bundle size significantly. I tried to send them a PR later for this.

https://github.com/MoOx/statinamic/compare/babel-node-loaders...thangngoc89:html-to-react?expand=1

For our codebase, there is 1 more thing I have to fix. It is about url prefix (aka basename)

Maybe we should use react-router's basename feature ? instead of prefix every url in docs with statinamic/

@MoOx
Copy link
Owner Author

MoOx commented Feb 8, 2016

basename as been introduced after I asked for it for statinamic, so maybe we should use it now :)

We will only need to to replace internal <a tags that matches the baseUrl (+pathname).

This code should definitely be in the core.

@thangngoc89
Copy link
Contributor

Haha. Didn't know about that

@thangngoc89
Copy link
Contributor

Is this a right approach? Should I make an PR for this after big pr landed?

@MoOx
Copy link
Owner Author

MoOx commented Feb 9, 2016

Looks like a good start since diff seems to be small.
I just hope the html2react thing is not too big because as you made this, it will be in the client side.

@thangngoc89
Copy link
Contributor

I raised an issue here https://github.com/mikenikles/html-to-react/issues/22

html2react only have 2 deps: htmlparser2 and lodash (they only use 4-5 functions of lodash)

@MoOx
Copy link
Owner Author

MoOx commented Feb 9, 2016

Anyway I think on the long term we will need to handle this before client side (because we can).
But that's a start :)

@thangngoc89
Copy link
Contributor

Ok. I'll send a PR then.

@thangngoc89
Copy link
Contributor

Thanks to gatsbyjs/gatsby#93

We can use these LoC to solve this issue:

import catchLinks from 'catch-links'
import { browserHistory } from 'react-router'

if (typeof window !== 'undefined') {
  catchLinks(window, (href) => {
    browserHistory.push(href)
  })
}

But I would love to have a proper implementation of this using remark, remark-react :)

@MoOx
Copy link
Owner Author

MoOx commented Feb 20, 2016

That's better than nothing. Will integrate in 0.8.0

@thangngoc89
Copy link
Contributor

'catch-links' have a bug with in-page anchor.

I have a fork of my own here : https://github.com/thangngoc89/blog/blob/e198ade146e5842b5175f938ddac6018be5563e2/web_modules/app/catchLinks.js

@thangngoc89
Copy link
Contributor

Forgot to mention that, the above function only work with website that doesn't use pathname.

For website that uses pathname (like docs page) you have to prepend pathname or use react-router basename

@MoOx
Copy link
Owner Author

MoOx commented Feb 21, 2016

Did you opened an issue on the original repo for this bug?

@thangngoc89
Copy link
Contributor

I planed to send a PR, just forgot.

@MoOx
Copy link
Owner Author

MoOx commented Feb 21, 2016

react-router does that already on links it handle, so maybe we should do this in markdown content only?

@MoOx MoOx removed the blocked label Feb 21, 2016
@thangngoc89
Copy link
Contributor

@MoOx
Copy link
Owner Author

MoOx commented Feb 22, 2016

We should add this logic in PageContainer to scope this in the dangerousHTML part.

@thangngoc89
Copy link
Contributor

@MoOx Will add it in #165

@MoOx MoOx mentioned this issue Feb 22, 2016
@MoOx
Copy link
Owner Author

MoOx commented Feb 24, 2016

Closed by 07aaa7b

@MoOx MoOx closed this as completed Feb 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants