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

Fix links on both github and website #576

Merged
merged 6 commits into from Feb 20, 2018
Merged

Conversation

azizhk
Copy link
Contributor

@azizhk azizhk commented Feb 18, 2018

A lot of links were broken on the website when trying to open them in a new tab. This is because they were relative urls like docs/css, so if you were on a pages like https://emotion.sh/docs/install then the url would become https://emotion.sh/docs/docs/css. Double /docs/
The urls were working if opened in the same tab because the gatsby-plugin-catch-links was somehow catching them correctly.

I ran http://www.brokenlinkcheck.com/ on https://emotion.sh and got the following 404s.
screen shot 2018-02-18 at 5 27 02 pm

My approach was that the links should work on github as well as on the website.
Like if you go to https://github.com/azizhk/emotion/blob/1bf4eba72a8e11660cb4dc7b7a4ed717afae9b58/docs/babel.md and click on the last link then it should work and take you to the corresponding markdown file in the repository.

.replace(/\.md(#.*)?$/, (match, hash) => {
return hash
})
.replace(/^\/packages\//, '/docs/')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the main code for creating proper urls is here.

- [css prop](https://github.com/emotion-js/emotion/tree/master/docs/css#CSS-Prop.md)
- [composition](https://github.com/emotion-js/emotion/tree/master/docs/composition.md)
- [keyframes](https://github.com/emotion-js/emotion/tree/master/docs/keyframes.md)
- [fontFace](https://github.com/emotion-js/emotion/tree/master/docs/font-face.md)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted fontFace, could not find appropriate url

@emmatown emmatown self-requested a review February 18, 2018 21:33
Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

This will break doc links when viewed on npm. The links should be in the format https://emotion.sh/docs/doc-name so that everything links to the site. The https://emotion.sh is stripped out here so that the links work on netlify previews and etc.

@azizhk
Copy link
Contributor Author

azizhk commented Feb 19, 2018

So that is the culprit, along with removing https://emotion.sh it is also removing one more slash.
Which results in wrong link for cx in https://emotion.sh/docs/composition
Should I change

node.url = node.url.replace(/^https?:\/\/emotion.sh\//, '')

to

node.url = node.url.replace(/^https?:\/\/emotion.sh/, '')

@azizhk
Copy link
Contributor Author

azizhk commented Feb 19, 2018

NPM also follows relative links when present in Readme.md of packages.
Example:
https://www.npmjs.com/package/zeroclipboard#other-limitations
https://github.com/zeroclipboard/zeroclipboard/blame/09660421dd377fda5bd66ce9bb69c916470bfbb5/README.md#L52

I've changed only one url from https://emotion.sh/* website to relative in babel-plugin-emotion's Readme.md, will revert that to point to the website.

And can you help me how to use netlify preview.

@emmatown
Copy link
Member

@azizhk Yep, make that change.

@azizhk
Copy link
Contributor Author

azizhk commented Feb 19, 2018

@tkh44
Copy link
Member

tkh44 commented Feb 19, 2018

Thank you!

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Could you run yarn lint and commit the changes so that prettier fixes the formatting?

@tkh44 tkh44 merged commit 015437f into emotion-js:master Feb 20, 2018
@azizhk
Copy link
Contributor Author

azizhk commented Feb 20, 2018

Hey yeah sorry about the lint errors, actually I somehow could not setup lerna properly.
I ran yarn install and then yarn bootstrap but somehow it did not install all dependencies and maybe that is why the githooks were not installed.

I cded to packages/site and then ran npm install to work on the website.
Don't know if anyone else is facing this issue.
Config:
macOS High Sierra
yarn -v: 1.3.2
node -v: v8.9.4
npm -v: 5.6.0

Also please need help with #573

@azizhk azizhk deleted the fix_links branch February 20, 2018 07:32
@emmatown
Copy link
Member

@azizhk you don't actually need to run yarn bootstrap anymore (we need to update the CONTRIBUTING.md), we don't have any githooks. What do you mean by it didn't install all the dependencies? What happened instead of everything being installed?

@azizhk
Copy link
Contributor Author

azizhk commented Feb 20, 2018

So I ran yarn start:site and it said it could not find babel-plugin-emotion.
I think the linking did not happen.
screen shot 2018-02-20 at 1 30 43 pm

Would you like a PR to add githooks? (Using husky & lint-staged)

@emmatown
Copy link
Member

@azizhk You need to run yarn build to build a version of babel-plugin-emotion that the site can use.

Personally, I'm not big on githooks because I find they get annoying since I already having linting in my editor but I wouldn't mind hearing @tkh44's opinion, I don't care that much so if @tkh44 thinks it's a good idea then sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants