-
Notifications
You must be signed in to change notification settings - Fork 582
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
Introduce markdown linting #2726
Changes from 20 commits
05cf1c7
9a0e810
399abc1
344c104
74eb624
3b6eb97
18cc5c0
353150c
d2591ff
e92701d
6675d93
a7a29e3
88b2216
ad55a36
f992a52
c17e0e0
f39fa4d
433324f
d741723
ef7242f
d1f87d2
572c744
5f27844
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,13 @@ | ||
{ | ||
"name": "Primer Components", | ||
"image": "mcr.microsoft.com/vscode/devcontainers/typescript-node:16", | ||
"extensions": ["esbenp.prettier-vscode", "dbaeumer.vscode-eslint"], | ||
"extensions": ["esbenp.prettier-vscode", "dbaeumer.vscode-eslint", "DavidAnson.vscode-markdownlint"], | ||
"forwardPorts": [8000], | ||
"postCreateCommand": ["/bin/bash", "-c", "pushd docs && npm install && popd && npm install"], | ||
"remoteUser": "node", | ||
"features": { | ||
"ghcr.io/devcontainers/features/sshd:1": { | ||
"version": "latest" | ||
"version": "latest" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
const githubMarkdownOpinions = require('@github/markdownlint-github') | ||
|
||
// Rules we want to turn on but currently have too many violations | ||
const rulesToEnforce = { | ||
'fenced-code-language': false, | ||
'no-duplicate-header': false, // Fix https://github.com/primer/doctocat/issues/527, then set this rule to `siblings_only: true` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @primer/react-reviewers FYI, Ideally, the issue is fixed in doctocat so we don't have to set this rule to be strict. |
||
} | ||
// Rules we don't care to enforce (usually stylistic) | ||
const rulesToNotEnforce = { | ||
'line-length': false, | ||
'blanks-around-headings': false, | ||
'blanks-around-lists': false, | ||
'no-trailing-spaces': false, | ||
'no-multiple-blanks': false, | ||
'no-trailing-punctuation': false, | ||
'single-trailing-newline': false, | ||
'ul-indent': false, | ||
'no-hard-tabs': false, | ||
'first-line-heading': false, | ||
'no-space-in-emphasis': false, | ||
'blanks-around-fences': false, | ||
} | ||
Comment on lines
+9
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are stylistic rules that I don't think Primer team cares to enforce for now, but not sure. Feel free to configure this to your preference!!! |
||
|
||
const defaultOverrides = { | ||
'no-generic-link-text': {exceptions: ['link']}, // We don't want it to flag links that link to `Link` component. | ||
} | ||
|
||
const options = githubMarkdownOpinions.init({...rulesToNotEnforce, ...rulesToEnforce, ...defaultOverrides}) | ||
module.exports = { | ||
config: options, | ||
customRules: ['@github/markdownlint-github'], | ||
outputFormatters: [ | ||
['markdownlint-cli2-formatter-pretty', {appendLink: true}], // ensures the error message includes a link to the rule documentation | ||
], | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,15 +12,16 @@ _Note: Consumer is used multiple times on this page. It refers to the developers | |
|
||
## Decision: | ||
|
||
|
||
1. Prefer using children for “content” | ||
|
||
2. For composite components, the API should be decided by how much customisation is available for children. | ||
|
||
For components that have design decisions baked in, should use strict props. For example, the color of the icon inside a Button component is decided by the `variant` prop on the Button. The API does not allow for changing that. | ||
|
||
```jsx | ||
<Button variant="danger" leadingIcon={TrashIcon}>Delete branch</Button> | ||
<Button variant="danger" leadingIcon={TrashIcon}> | ||
Delete branch | ||
</Button> | ||
``` | ||
|
||
On the other hand, if we want consumers to have more control over children, a composite API is the better choice. | ||
|
@@ -38,7 +39,7 @@ On the other hand, if we want consumers to have more control over children, a co | |
|
||
With React, `children` is the out-of-the-box way for putting [phrasing content](https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_categories#phrasing_content) inside your component. By using `children` instead of our own custom prop, we can make the API “predictable” for its consumers. | ||
|
||
<img width="373" alt="image" src="https://user-images.githubusercontent.com/1863771/144945223-70c4c800-5827-4985-9f18-0ab416eba058.png"> | ||
<img width="373" alt="image" src="https://user-images.githubusercontent.com/1863771/144945223-70c4c800-5827-4985-9f18-0ab416eba058.png"> <!-- markdownlint-disable-line no-default-alt-text --> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @primer/react-reviewers This is a TODO! Feel free to suggest something in this PR to fix this! |
||
|
||
```jsx | ||
// prefer this | ||
|
@@ -62,7 +63,7 @@ import {CheckIcon} from '@primer/octicons-react' | |
render( | ||
<Flash variant="success"> | ||
<CheckIcon /> Changes saved! | ||
</Flash> | ||
</Flash>, | ||
) | ||
``` | ||
|
||
|
@@ -149,8 +150,6 @@ When intentionally going off the happy path, developers can still drop down an a | |
|
||
_Sidenote: We might want to name this prop `leadingIcon`, even if there is no `trailingIcon`. Consistent names across components plays a big role in making the API predictable._ | ||
|
||
|
||
|
||
--- | ||
|
||
<br/> | ||
|
@@ -233,7 +232,7 @@ We use this pattern as well in `Button`, `Button.Counter` is a restricted versio | |
|
||
<img width="184" alt="image 8" src="https://user-images.githubusercontent.com/1863771/144945218-5154b8a1-8854-4335-926c-08a4ffac6d9d.png"> | ||
|
||
```jsx | ||
````jsx | ||
<Button> | ||
Watch <Button.Counter>1</Button.Counter> | ||
</Button> | ||
|
@@ -259,7 +258,7 @@ For Example, [legacy ActionMenu](https://primer.style/react/deprecated/ActionMen | |
```jsx | ||
<ActionMenu overlayProps={{width: 'medium'}} anchorContent="Open column menu"> | ||
</ActionMenu> | ||
``` | ||
```` | ||
|
||
<br/> | ||
|
||
|
@@ -371,15 +370,13 @@ Prefer using children for “content” | |
<Button label="Watch" variant="primary"/> | ||
``` | ||
|
||
|
||
<img width="227" alt="image 13" src="https://user-images.githubusercontent.com/1863771/145045542-0d80491b-75e1-4304-b9fe-8c2cca80b298.png"> | ||
|
||
|
||
The Icon should adapt to variant and size of the `Button`. We could use a `EyeIcon` in children here: | ||
|
||
```jsx | ||
<Button> | ||
<EyeIcon/> Watch | ||
<EyeIcon /> Watch | ||
</Button> | ||
``` | ||
|
||
|
@@ -393,10 +390,8 @@ But, we want to discourage customising the Icon’s color and size in the applic | |
<Button leadingIcon={<EyeIcon/>}>Watch</Button> | ||
``` | ||
|
||
|
||
<img width="293" alt="image 14" src="https://user-images.githubusercontent.com/1863771/145045544-1a1651f1-fbcf-4022-8e9b-b37558bb2466.png"> | ||
|
||
|
||
We want to add a `Counter` that adapts to the variant without supporting all the props of a `CounterLabel` like `scheme`. | ||
|
||
`Button.Counter` is a restricted version of `CounterLabel`, making the right thing easy and wrong thing hard: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,44 +10,44 @@ In Primer React and consuming applications, we use many different patterns for c | |
|
||
1. Creating components with styled-components | ||
|
||
```tsx | ||
const Avatar = styled.img.attrs<StyledAvatarProps>(props => ({ | ||
height: props.size, | ||
width: props.size | ||
}))<StyledAvatarProps>` | ||
display: inline-block; | ||
overflow: hidden; | ||
line-height: ${get('lineHeights.condensedUltra')}; | ||
border-radius: ${props => getBorderRadius(props)}; | ||
${sx} | ||
` | ||
``` | ||
|
||
[Show full code example →](https://github.com/primer/react/pull/2019/files?diff=split&w=0) | ||
|
||
2. Creating components with Box | ||
|
||
```tsx | ||
const Avatar: React.FC<AvatarProps> = ({size = 20, alt = '', square = false, sx = {}, ...props}) => { | ||
const styles:BetterSystemStyleObject = { | ||
display: 'inline-block', | ||
overflow: 'hidden', | ||
lineHeight: 'condensedUltra', | ||
borderRadius: getBorderRadius({size, square}) | ||
} | ||
|
||
return ( | ||
<Box | ||
as="img" | ||
alt={alt} | ||
sx={merge<BetterSystemStyleObject>(styles, sx)} // styles needs to merge with props.sx | ||
{...props} | ||
/> | ||
) | ||
} | ||
``` | ||
|
||
[Show full code example →](https://github.com/primer/react/pull/2019/files?diff=split&w=0) | ||
```tsx | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errr I think prettier ran for these files and updated it, but not sure. @primer/react-reviewers is this expected? |
||
const Avatar = styled.img.attrs<StyledAvatarProps>(props => ({ | ||
height: props.size, | ||
width: props.size, | ||
}))<StyledAvatarProps>` | ||
display: inline-block; | ||
overflow: hidden; | ||
line-height: ${get('lineHeights.condensedUltra')}; | ||
border-radius: ${props => getBorderRadius(props)}; | ||
${sx} | ||
` | ||
``` | ||
|
||
[Show full code example →](https://github.com/primer/react/pull/2019/files?diff=split&w=0) | ||
|
||
2. Creating components with Box | ||
|
||
```tsx | ||
const Avatar: React.FC<AvatarProps> = ({size = 20, alt = '', square = false, sx = {}, ...props}) => { | ||
const styles: BetterSystemStyleObject = { | ||
display: 'inline-block', | ||
overflow: 'hidden', | ||
lineHeight: 'condensedUltra', | ||
borderRadius: getBorderRadius({size, square}), | ||
} | ||
|
||
return ( | ||
<Box | ||
as="img" | ||
alt={alt} | ||
sx={merge<BetterSystemStyleObject>(styles, sx)} // styles needs to merge with props.sx | ||
{...props} | ||
/> | ||
) | ||
} | ||
``` | ||
|
||
[Show full code example →](https://github.com/primer/react/pull/2019/files?diff=split&w=0) | ||
|
||
| ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add this to the
lint
job as a step afternpm run lint
? Would be great to keep it in the same job for linting, something like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can do that! I got feedback in the
primer/design
repo that it's more clear when there are separate linting jobs for error clarity, but we can keep it together if you prefer. I don't have a strong opinion.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!