MoOx / phenomic

DEPRECATED. Please use Next.js instead.
https://nextjs.org
MIT License
3.21k stars 249 forks source link

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

Closed MoOx closed 8 years ago

MoOx commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Haha. Didn't know about that

thangngoc89 commented 8 years ago

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

MoOx commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Ok. I'll send a PR then.

thangngoc89 commented 8 years ago

Thanks to https://github.com/gatsbyjs/gatsby/issues/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 commented 8 years ago

That's better than nothing. Will integrate in 0.8.0

thangngoc89 commented 8 years ago

'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 commented 8 years ago

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 commented 8 years ago

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

thangngoc89 commented 8 years ago

I planed to send a PR, just forgot.

MoOx commented 8 years ago

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

thangngoc89 commented 8 years ago

Yes. I'm doing that. https://github.com/thangngoc89/blog/blob/master/web_modules/app/routes.js#L10-L18

MoOx commented 8 years ago

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

thangngoc89 commented 8 years ago

@MoOx Will add it in #165

MoOx commented 8 years ago

Closed by 07aaa7be263453b76e664792919b132ba7159db8