-
Notifications
You must be signed in to change notification settings - Fork 70
chore: update documentation for project guidelines and standards #4270
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
Conversation
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.
Pull request overview
This PR restructures and significantly expands the project's documentation guidelines to provide clearer, more actionable guidance for developers. The documentation is transformed from simple bullet-point lists into comprehensive, well-structured guides with concrete examples.
Key changes:
- Enhanced formatting with code examples demonstrating good and bad patterns
- Organized content into clear sections with visual hierarchy
- Added specific commands and practical instructions
- Standardized structure across all guideline documents
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
.github/instructions/ts.instructions.md |
Expanded TypeScript guidelines with type definition patterns, functional programming principles, and comprehensive testing examples |
.github/instructions/styling.instructions.md |
Restructured CSS guidelines with BEM examples, responsive design patterns, and token usage |
.github/instructions/react.instructions.md |
Enhanced React component structure documentation with file organization, implementation patterns, and accessibility requirements |
.github/instructions/global-coding.instructions.md |
Improved global standards with formatted tables, practical commands, and clearer naming convention examples |
.github/instructions/figma.instructions.md |
Expanded Figma workflow with numbered steps, anti-patterns, and a checklist |
.github/copilot-instructions.md |
Simplified overview with streamlined package descriptions and guideline references |
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
8b278e1 to
9958ee6
Compare
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
| import { MyComponentProps } from './MyComponent.types'; | ||
| import './my-component.css'; |
Copilot
AI
Dec 8, 2025
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.
Missing semicolons in import statements. For consistency with TypeScript best practices, add semicolons after each import statement.
| export const MyComponent: React.FC<MyComponentProps> = ({ | ||
| children, | ||
| variant = 'default', | ||
| ...props | ||
| }) => { | ||
| return ( | ||
| <div className="my-component" {...props}> | ||
| {children} | ||
| </div> | ||
| ); | ||
| }; | ||
| ``` |
Copilot
AI
Dec 8, 2025
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.
Missing semicolons at the end of the export statement and function body. Add semicolons for consistency with TypeScript best practices.
| export const Button = () => {} | ||
| export const useButton = () => {} | ||
|
|
||
| // ❌ Avoid default exports | ||
| export default Button | ||
|
|
||
| // ✅ Import named exports | ||
| import { Button, useButton } from '@equinor/eds-core-react' | ||
|
|
||
| // ❌ Don't import default | ||
| import Button from '@equinor/eds-core-react' |
Copilot
AI
Dec 8, 2025
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.
Missing semicolons in export and import examples. For consistency with TypeScript best practices, add semicolons after each statement.
| export const Button = () => {} | |
| export const useButton = () => {} | |
| // ❌ Avoid default exports | |
| export default Button | |
| // ✅ Import named exports | |
| import { Button, useButton } from '@equinor/eds-core-react' | |
| // ❌ Don't import default | |
| import Button from '@equinor/eds-core-react' | |
| export const Button = () => {}; | |
| export const useButton = () => {}; | |
| // ❌ Avoid default exports | |
| export default Button; | |
| // ✅ Import named exports | |
| import { Button, useButton } from '@equinor/eds-core-react'; | |
| // ❌ Don't import default | |
| import Button from '@equinor/eds-core-react'; |
| **Rules:** | ||
|
|
||
| - No conditional hooks (move into separate components if needed) | ||
| - No React import needed (no JSX pragma in modern React) |
Copilot
AI
Dec 8, 2025
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.
The comment "No React import needed (no JSX pragma in modern React)" might be confusing. While it's true that React 17+ with the new JSX transform doesn't require importing React for JSX, the example code at lines 31-42 doesn't show any React import either. Consider clarifying this applies to JSX usage specifically, or show both cases (with and without JSX).
| - No React import needed (no JSX pragma in modern React) | |
| - For files using JSX with React 17+ and the new JSX transform, no React import is needed. | |
| For non-JSX usage (e.g., using React APIs directly), you still need to import React. | |
| Example (JSX, no import needed): | |
| ```typescript | |
| // React 17+ with new JSX transform | |
| export const MyComponent = () => <div>Hello</div>; |
Example (non-JSX, import needed):
import * as React from 'react';
// Using React.createElement directly
export const MyComponent = () => React.createElement('div', null, 'Hello');| disabled?: boolean | ||
| } | ||
|
|
||
| const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {} |
Copilot
AI
Dec 8, 2025
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.
Missing semicolon at end of statement. For consistency with other examples in the documentation and TypeScript best practices, add a semicolon: const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {};
| const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {} | |
| const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {}; |
|
|
||
| // ❌ Avoid | ||
| type ButtonVariant = string | ||
| const handleClick = (event: any) => {} |
Copilot
AI
Dec 8, 2025
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.
Missing semicolon at end of statement. For consistency, add a semicolon: const handleClick = (event: any) => {};
| const handleClick = (event: any) => {} | |
| const handleClick = (event: any) => {}; |
| render(<Button disabled>Loading</Button>); | ||
| const button = screen.getByRole('button'); | ||
| expect(button).toBeDisabled(); | ||
| }); |
Copilot
AI
Dec 8, 2025
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.
Missing semicolons in the test example. For consistency with TypeScript best practices, add semicolons after each statement.
| padding: 0.5rem 1rem; | ||
| } | ||
|
|
||
| @media (min-width: 768px) { | ||
| .button { | ||
| padding: 0.75rem 1.5rem; | ||
| } |
Copilot
AI
Dec 8, 2025
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.
Missing semicolons in CSS property declarations. Add semicolons after each property for consistency.
| color: var(--eds-color-text-strong); | ||
| background: var(--eds-color-bg-input); | ||
| border: 1px solid var(--eds-color-border-medium); | ||
| } |
Copilot
AI
Dec 8, 2025
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.
Missing semicolons in CSS property declarations. While CSS semicolons at the end of the last property in a block are optional, it's a best practice to include them for consistency and to prevent errors when adding new properties. Add semicolons after each property.
Co-authored-by: Copilot <[email protected]>
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
No description provided.