-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat(components): Co-Pilot Instructions First Pass #2294
base: master
Are you sure you want to change the base?
Changes from 1 commit
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 |
---|---|---|
@@ -0,0 +1,76 @@ | ||
1. **TypeScript**: | ||
|
||
- Use TypeScript, specifically version 5.0+. | ||
- Ensure type safety and leverage TypeScript features. | ||
- Use function(){} style definition, and not () => {} | ||
- Prefer named exports over default | ||
- When defining interfaces, we provide descriptions in JSDoc format for each | ||
property | ||
|
||
2. **CSS Modules**: | ||
|
||
- Use CSS Modules for styles. | ||
- Import styles using a default import named `styles`. | ||
|
||
3. **Code Style**: | ||
|
||
- Use double quotes instead of single quotes. | ||
- Follow consistent code formatting and style guidelines. | ||
|
||
4. **Dependency Management**: | ||
|
||
- Use npm to manage dependencies. | ||
|
||
5. **Accessibility**: | ||
|
||
- Consider accessibility in all code. | ||
- Use ARIA roles and properties where applicable. | ||
|
||
6. **Comments and Documentation**: | ||
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. Is there a way to train it on our own guides for docs? Like
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. Unfortunately not! They explicitly call that out as something that doesn't work: https://docs.github.com/en/copilot/customizing-copilot/adding-custom-instructions-for-github-copilot#writing-effective-custom-instructions |
||
|
||
- Include quality comments and documentation in all code. | ||
- Provide usage examples and prop descriptions for components. | ||
|
||
7. **Testing**: | ||
- Use Jest for testing. | ||
- Use Testing Library within Jest for testing React components. | ||
|
||
### Example Implementation | ||
|
||
#### TypeScript Component with CSS Modules and Accessibility | ||
|
||
```tsx | ||
import React from "react"; | ||
import styles from "./YourComponent.module.css"; | ||
|
||
/** | ||
* YourComponent is a sample component that demonstrates the use of TypeScript, | ||
* CSS Modules, and accessibility best practices. | ||
* | ||
* @param {object} props - The component props. | ||
* @param {string} props.label - The label for the component. | ||
* @returns {JSX.Element} The rendered component. | ||
*/ | ||
export function YourComponent({ label }: { label: string }): JSX.Element { | ||
return ( | ||
<div className={styles.container} role="region" aria-label={label}> | ||
<label className={styles.label}>{label}</label> | ||
<input className={styles.input} type="text" aria-labelledby={label} /> | ||
</div> | ||
); | ||
} | ||
``` | ||
|
||
#### Jest Test with Testing Library | ||
|
||
```tsx | ||
import React from "react"; | ||
import { render, screen } from "@testing-library/react"; | ||
import YourComponent from "./YourComponent"; | ||
|
||
test("renders YourComponent with label", () => { | ||
render(<YourComponent label="Test Label" />); | ||
const labelElement = screen.getByText(/Test Label/i); | ||
expect(labelElement).toBeInTheDocument(); | ||
}); | ||
``` |
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.
Maybe "prefer HTML-native roles and attributes, and use ARIA roles only as necessary" or something? I guess "where applicable" already gets at that, but I wonder how the LLM reads that in practice (ie will it take that as a sign to prefer ARIA roles?)
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.
Done!