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

All markdown files are properly formatted now #181

Merged
merged 8 commits into from Oct 17, 2019

Conversation

@nickytonline
Copy link
Contributor

nickytonline commented Oct 11, 2019

This PR implements #162, and adds a precommit hook via lint-staged and husky. For the time being, I have only added an entry to lint-staged to format markdown (md) files. As well, all the md files have been formatted using the prettier settings that I copied from Storybook's settings.

Built everything locally and it looks good. I just did a smoke test navigating to some pages.

learnstorybook.com screeshot

Also, Skeletor approves of this PR.

Skeletor approves

@@ -0,0 +1,2 @@
yarn.lock

This comment has been minimized.

Copy link
@nickytonline

nickytonline Oct 11, 2019

Author Contributor

We never want prettier to format these files.

.prettierrc Outdated
@@ -0,0 +1,7 @@
{

This comment has been minimized.

This comment has been minimized.

Copy link
@kylesuss

kylesuss Oct 11, 2019

Member

Is it possible to make this formatting work with the prettier config that we have already here?

'prettier/prettier': [
'warn',
{
printWidth: 100,
tabWidth: 2,
bracketSpacing: true,
trailingComma: 'es5',
singleQuote: true,
jsxBracketSameLine: false,
},
],

The eslint config is already setup with these params.

This comment has been minimized.

Copy link
@nickytonline

nickytonline Oct 11, 2019

Author Contributor

Oops, didn’t see that. I will update it later tonight.

@@ -40,6 +40,17 @@
"storybook": "start-storybook -s ./static -p 6006",
"test": "echo \"Error: no test specified\" && exit 1"
},
"husky": {

This comment has been minimized.

Copy link
@nickytonline

nickytonline Oct 11, 2019

Author Contributor

The precommit magic.

@nickytonline nickytonline changed the title All markdown files are formatted properly now All markdown files are properly formatted now Oct 11, 2019
}
},
"lint-staged": {
"*.md": [

This comment has been minimized.

Copy link
@nickytonline

nickytonline Oct 11, 2019

Author Contributor

Other file types can be added here in another PR.

jsxBracketSameLine: false,
},
],
'prettier/prettier': ['warn', prettierConfig],

This comment has been minimized.

Copy link
@nickytonline

nickytonline Oct 12, 2019

Author Contributor

This is imported now, so that files that are non-JS can use the same prettier config.

This comment has been minimized.

Copy link
@nickytonline

nickytonline Oct 12, 2019

Author Contributor

I am curious as to why eslint is configured to only warn about prettier. I imagine once a config for JS files is added to the lint-staged configuration, this would no longer be required?

This comment has been minimized.

Copy link
@kylesuss

kylesuss Oct 15, 2019

Member

I think the initial logic was that we didn't want to discourage people from contributing because of it. But yeah, given the changes you have implemented here which help reinforce the standards, I think we can change it eventually.

@@ -0,0 +1,8 @@
module.exports = {

This comment has been minimized.

Copy link
@nickytonline

nickytonline Oct 12, 2019

Author Contributor

This is the config that get's imported into .eslintrc.js

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Oct 12, 2019

Alright, I updated things to use the existing prettier config. Let me know if that's all good. That was the first time I've seen a prettier config in an eslint config which is why I didn't think to look there. ;)

Copy link
Member

kylesuss left a comment

I like the idea of sharing the config! I'm not sure if eslint can handle formatting a MD file w/ prettier given that it is a tool designed for JS files, so it seems like the route you have taken here is to have the formatting of the MD files happen during the commit cycle. I think that will work well, however I feel there is a small disconnect still --

For instance, there is this file:

https://github.com/nickytonline/learnstorybook.com/blob/md-docs-formatting/content/design-systems-for-developers/index.md

It appears to not have yet been formatted like the other MD files in this PR (the obvious sign is the double quotes). I would hope that adding & committing a change to the repo formats that file given the new husky setup, but that doesn't happen 🤔

Any ideas?

Very appreciative of you looking into this for us 🙇

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Oct 14, 2019

Yeah, sharing seemed to make the most sense to me for the prettier config.

I’ll rerun the batch prettier formatting for the non-prettier formatted *.md files. I think I used the wrong glob when I ran it, so it didn’t format all the files. I’ll update the PR this afternoon.

And as you mentioned, the only way for markdown files to get formatted is on the precommit hook. If you have an editor like VS Code you can have them formatted on save with the prettier extension, but there is no guarantee that everyone contributing would be using VS Code, so the precommit hook is a fail safe.

As mentioned previously, once this PR is merged, we can use the precommit hook for other file types as well: *.jsx, *.js, *.json etc.

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Oct 15, 2019

So the reason that particular file would not prettify is because of the location of the array square brackets for the contributors section. I lined them up with those of the authors section and then prettier ran as usual. That was not obvious. It looks like that is the only file that wasn't prettified. I'll push that change momentarily.

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Oct 15, 2019

Hmm even with that fix there is still parts that did not convert to single quotes. I'll continue to investigate.

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Oct 15, 2019

Ahh, I see what it is. It's mentioned in the prettier docs. See https://github.com/prettier/prettier/tree/21733e441dc01f7d85b483edee92b7e7507bfd9a#quotes. It's by design.

If the number of quotes outweighs the other quote, the quote which is less used will be used to format the string - Example: "I'm double quoted" results in "I'm double quoted" and "This \"example\" is single quoted" results in 'This "example" is single quoted'.

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Oct 15, 2019

I went through the other markdown files that have double quotes, and they are either respecting the rule I mentioned previously or they are markup and stay as double quotes instead of single quotes.

heroDescription: "A guide that teaches professional developers how to transform component libraries into design systems and set up the production infrastructure used by leading frontend teams."
title: 'Design Systems for Developers'
description: 'Discover how to build and maintain design systems using Storybook.'
heroDescription: 'A guide that teaches professional developers how to transform component libraries into design systems and set up the production infrastructure used by leading frontend teams.'
overview: "Design systems power the frontend teams of Shopify, IBM, Salesforce, Airbnb, Twitter, and many more. This guide for professional developers examines how the smartest teams engineer design systems at scale and why they use the tools they use. We'll walk through setting up core services, libraries, and workflows to develop a design system from scratch."

This comment has been minimized.

Copy link
@nickytonline

nickytonline Oct 15, 2019

Author Contributor

As mentioned in the main convo, this is how prettier works. Since there is a single quote in the text, it will keep the double quotes for the string.

@@ -0,0 +1,2 @@
yarn.lock
package-lock.json

This comment has been minimized.

Copy link
@kylesuss

kylesuss Oct 15, 2019

Member

Nice. Haha we aren't supposed to have both of these lockfiles, so I will submit a small PR to remove the package-lock.json.

commit: '8db511e'
---

上一章我們構建了第一個元件; 本章 我們學習 擴充套件構建TaskList的任務列表. 讓我們將 元件組合 在一起,看看在引入更多複雜性時會發生什麼.
上一章我們構建了第一個元件; 本章 我們學習 擴充套件構建 TaskList 的任務列表. 讓我們將 元件組合 在一起,看看在引入更多複雜性時會發生什麼.

This comment has been minimized.

Copy link
@kylesuss

kylesuss Oct 15, 2019

Member

@danielduan or @chinanf-boy -- pinging you here since you contributed the original Chineses translations. Prettier is formatting some of these files slightly differently. I recognize there are 2 Chinese translations and it appears to happen in both of them.

Seems like the common change is to insert a space around English words and Chinese characters. So:

擴充套件構建TaskList的任務列表

becomes

擴充套件構建 TaskList 的任務列表

The same happens in the zh-CN translation. Does this "break" the translation at all? Or is it an acceptable change? If it breaks things, I think we can probably ignore these files in Prettier.

Thanks 🙇

Copy link
Member

kylesuss left a comment

@nickytonline ok I just merged #188 which gets rid of the package lock as well as adds it to the gitignore. Mind rebasing & getting both that conflict as well as the es translation conflict sorted?

After that, I am all good on this PR once I get confirmation here regarding the Chinese translations -- I did a run through locally for the prettier behavior and all seems great. Thanks again for working through this 💯

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Oct 15, 2019

Sounds good. I’ll get to this later this evening.

@nickytonline nickytonline force-pushed the nickytonline:md-docs-formatting branch from ceaffc2 to 67bb8b7 Oct 15, 2019
@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Oct 15, 2019

Alright, I've rebased and fixed the merge conflicts @kylesuss. So this should be good to go. As you mentioned, just waiting to hear about the Chinese translations.

@domyen

This comment has been minimized.

Copy link
Member

domyen commented Oct 17, 2019

@kylesuss @nickytonline I suggest we move forward with auto-formatting Chinese. According to this article it's fine to add a space between English words and Chinese characters.

@kylesuss

This comment has been minimized.

Copy link
Member

kylesuss commented Oct 17, 2019

@domyen nice find. Interestingly, that article seems to imply that the old way was wrong! Ha. Yes let's move forward on this and circle back if need be.

Copy link
Member

kylesuss left a comment

Thanks for the patience & effort @nickytonline !!

@kylesuss kylesuss merged commit 096f288 into chromaui:master Oct 17, 2019
1 check passed
1 check passed
netlify/tender-kilby-05eed0/deploy-preview Deploy preview ready!
Details
@nickytonline nickytonline deleted the nickytonline:md-docs-formatting branch Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.