You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2025/12/11 07:31:06 UTC
(superset) branch docs/contribution-guidelines-from-wiki created (now d2b5df55e1)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a change to branch docs/contribution-guidelines-from-wiki
in repository https://gitbox.apache.org/repos/asf/superset.git
at d2b5df55e1 docs: add contribution guidelines from wiki to Developer Portal
This branch includes the following new commits:
new d2b5df55e1 docs: add contribution guidelines from wiki to Developer Portal
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/01: docs: add contribution guidelines from wiki to Developer Portal
Posted by ru...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch docs/contribution-guidelines-from-wiki
in repository https://gitbox.apache.org/repos/asf/superset.git
commit d2b5df55e1ffea4048e68d2fa3e905139895a279
Author: Evan Rusackas <ev...@rusackas.com>
AuthorDate: Wed Dec 10 23:22:32 2025 -0800
docs: add contribution guidelines from wiki to Developer Portal
- Add Design Guidelines (capitalization, buttons, error messages)
- Add Frontend Style Guidelines with sub-pages:
- Component Style Guidelines and Best Practices
- Emotion Styling Guidelines and Best Practices
- Add Backend Style Guidelines with sub-pages:
- DAO Style Guidelines and Best Practices
- Move Testing Guidelines to Testing section
- Consolidate sidebar structure (remove nested "Contribution Guidelines")
- Rename duplicate section titles to "Overview"
Content sourced from Apache Superset wiki pages.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <no...@anthropic.com>
---
.../guidelines/backend-style-guidelines.md | 36 +-
.../guidelines/backend/dao-style-guidelines.md | 24 +-
.../guidelines/design-guidelines.md | 131 +++++--
.../guidelines/frontend-style-guidelines.md | 42 +-
.../frontend/component-style-guidelines.md | 250 +++++-------
.../frontend/emotion-styling-guidelines.md | 425 +++++++++------------
.../guidelines/frontend/testing-guidelines.md | 297 --------------
.../developer_portal/testing/testing-guidelines.md | 129 +++++++
docs/sidebarTutorials.js | 37 +-
9 files changed, 582 insertions(+), 789 deletions(-)
diff --git a/docs/developer_portal/guidelines/backend-style-guidelines.md b/docs/developer_portal/guidelines/backend-style-guidelines.md
index cb046f893d..d938298d8a 100644
--- a/docs/developer_portal/guidelines/backend-style-guidelines.md
+++ b/docs/developer_portal/guidelines/backend-style-guidelines.md
@@ -1,6 +1,6 @@
---
-title: Backend Style Guidelines
-sidebar_position: 3
+title: Overview
+sidebar_position: 1
---
<!--
@@ -26,22 +26,22 @@ under the License.
This is a list of statements that describe how we do backend development in Superset. While they might not be 100% true for all files in the repo, they represent the gold standard we strive towards for backend quality and style.
-* We use a monolithic Python/Flask/Flask-AppBuilder backend, with small single-responsibility satellite services where necessary.
-* Files are generally organized by feature or object type. Within each domain, we can have api controllers, models, schemas, commands, and data access objects (dao).
- * See: [Proposal for Improving Superset's Python Code Organization](https://github.com/apache/superset/issues/9077)
-* API controllers use Marshmallow Schemas to serialize/deserialize data.
-* Authentication and authorization are controlled by the [security manager](https://github.com/apache/superset/blob/master/superset/security/manager).
-* We use Pytest for unit and integration tests. These live in the `tests` directory.
-* We add tests for every new piece of functionality added to the backend.
-* We use pytest fixtures to share setup between tests.
-* We use sqlalchemy to access both Superset's application database, and users' analytics databases.
-* We make changes backwards compatible whenever possible.
- * If a change cannot be made backwards compatible, it goes into a major release.
- * See: [Proposal For Semantic Versioning](https://github.com/apache/superset/issues/12566)
-* We use Swagger for API documentation, with docs written inline on the API endpoint code.
-* We prefer thin ORM models, putting shared functionality in other utilities.
-* Several linters/checkers are used to maintain consistent code style and type safety: pylint, pypy, black, isort.
-* `__init__.py` files are kept empty to avoid implicit dependencies.
+- We use a monolithic Python/Flask/Flask-AppBuilder backend, with small single-responsibility satellite services where necessary.
+- Files are generally organized by feature or object type. Within each domain, we can have api controllers, models, schemas, commands, and data access objects (DAO).
+ - See: [Proposal for Improving Superset's Python Code Organization](https://github.com/apache/superset/issues/9077)
+- API controllers use Marshmallow Schemas to serialize/deserialize data.
+- Authentication and authorization are controlled by the [security manager](https://github.com/apache/superset/blob/master/superset/security/manager).
+- We use Pytest for unit and integration tests. These live in the `tests` directory.
+- We add tests for every new piece of functionality added to the backend.
+- We use pytest fixtures to share setup between tests.
+- We use SQLAlchemy to access both Superset's application database, and users' analytics databases.
+- We make changes backwards compatible whenever possible.
+ - If a change cannot be made backwards compatible, it goes into a major release.
+ - See: [Proposal For Semantic Versioning](https://github.com/apache/superset/issues/12566)
+- We use Swagger for API documentation, with docs written inline on the API endpoint code.
+- We prefer thin ORM models, putting shared functionality in other utilities.
+- Several linters/checkers are used to maintain consistent code style and type safety: pylint, mypy, black, isort.
+- `__init__.py` files are kept empty to avoid implicit dependencies.
## Code Organization
diff --git a/docs/developer_portal/guidelines/backend/dao-style-guidelines.md b/docs/developer_portal/guidelines/backend/dao-style-guidelines.md
index 210c070aa4..fa0145a08a 100644
--- a/docs/developer_portal/guidelines/backend/dao-style-guidelines.md
+++ b/docs/developer_portal/guidelines/backend/dao-style-guidelines.md
@@ -1,6 +1,6 @@
---
title: DAO Style Guidelines and Best Practices
-sidebar_position: 1
+sidebar_position: 2
---
<!--
@@ -26,19 +26,29 @@ under the License.
A Data Access Object (DAO) is a pattern that provides an abstract interface to the SQLAlchemy Object Relational Mapper (ORM). The DAOs are critical as they form the building block of the application which are wrapped by the associated commands and RESTful API endpoints.
-Currently there are numerous inconsistencies and violation of the DRY principal within the codebase as it relates to DAOs and ORMs—unnecessary commits, non-ACID transactions, etc.—which makes the code unnecessarily complex and convoluted. Addressing the underlying issues with the DAOs _should_ help simplify the downstream operations and improve the developer experience.
+There are numerous inconsistencies and violations of the DRY principal within the codebase as it relates to DAOs and ORMs—unnecessary commits, non-ACID transactions, etc.—which makes the code unnecessarily complex and convoluted. Addressing the underlying issues with the DAOs _should_ help simplify the downstream operations and improve the developer experience.
To ensure consistency the following rules should be adhered to:
-1. All database operations (including testing) should be defined within a DAO, i.e., there should not be any explicit `db.session.add`, `db.session.merge`, etc. calls outside of a DAO.
+## Core Rules
-2. A DAO should use `create`, `update`, `delete`, `upsert` terms—typical database operations which ensure consistency with commands—rather than action based terms like `save`, `saveas`, `override`, etc.
+1. **All database operations (including testing) should be defined within a DAO**, i.e., there should not be any explicit `db.session.add`, `db.session.merge`, etc. calls outside of a DAO.
-3. Sessions should be managed via a [context manager](https://docs.sqlalchemy.org/en/20/orm/session_transaction.html#begin-once) which auto-commits on success and rolls back on failure, i.e., there should be no explicit `db.session.commit` or `db.session.rollback` calls within the DAO.
+2. **A DAO should use `create`, `update`, `delete`, `upsert` terms**—typical database operations which ensure consistency with commands—rather than action based terms like `save`, `saveas`, `override`, etc.
-4. There should be a single atomic transaction representing the entirety of the operation, i.e., when creating a dataset with associated columns and metrics either all the changes succeed when the transaction is committed, or all the changes are undone when the transaction is rolled back. SQLAlchemy supports [nested transactions](https://docs.sqlalchemy.org/en/20/orm/session_transaction.html#nested-transaction) via the `begin_nested` method which can be nested—inline with how DAOs are invoked.
+3. **Sessions should be managed via a [context manager](https://docs.sqlalchemy.org/en/20/orm/session_transaction.html#begin-once)** which auto-commits on success and rolls back on failure, i.e., there should be no explicit `db.session.commit` or `db.session.rollback` calls within the DAO.
-5. The database layer should adopt a "shift left" mentality i.e., uniqueness/foreign key constraints, relationships, cascades, etc. should all be defined in the database layer rather than being enforced in the application layer.
+4. **There should be a single atomic transaction representing the entirety of the operation**, i.e., when creating a dataset with associated columns and metrics either all the changes succeed when the transaction is committed, or all the changes are undone when the transaction is rolled back. SQLAlchemy supports [nested transactions](https://docs.sqlalchemy.org/en/20/orm/session_transaction.html#nested-transaction) via the `begin_nested` method which can be nested—inline with how DAOs ar [...]
+
+5. **The database layer should adopt a "shift left" mentality** i.e., uniqueness/foreign key constraints, relationships, cascades, etc. should all be defined in the database layer rather than being enforced in the application layer.
+
+6. **Exception-based validation**: Ask for forgiveness rather than permission. Try to perform the operation and rely on database constraints to verify that the model is acceptable, rather than pre-validating conditions.
+
+7. **Bulk operations**: Provide bulk `create`, `update`, and `delete` methods where applicable for performance optimization.
+
+8. **Sparse updates**: Updates should only modify explicitly defined attributes.
+
+9. **Test transactions**: Tests should leverage nested transactions which should be rolled back on teardown, rather than deleting objects.
## DAO Implementation Examples
diff --git a/docs/developer_portal/guidelines/design-guidelines.md b/docs/developer_portal/guidelines/design-guidelines.md
index 15167b550e..7bf96b1cca 100644
--- a/docs/developer_portal/guidelines/design-guidelines.md
+++ b/docs/developer_portal/guidelines/design-guidelines.md
@@ -30,21 +30,23 @@ This is an area to host resources and documentation supporting the evolution and
### Sentence case
-Use sentence-case capitalization for everything in the UI (except these **).
+Use sentence-case capitalization for everything in the UI (except these exceptions below).
Sentence case is predominantly lowercase. Capitalize only the initial character of the first word, and other words that require capitalization, like:
-* **Proper nouns.** Objects in the product _are not_ considered proper nouns e.g. dashboards, charts, saved queries etc. Proprietary feature names eg. SQL Lab, Preset Manager _are_ considered proper nouns
-* **Acronyms** (e.g. CSS, HTML)
-* When referring to **UI labels that are themselves capitalized** from sentence case (e.g. page titles - Dashboards page, Charts page, Saved queries page, etc.)
-* User input that is reflected in the UI. E.g. a user-named a dashboard tab
+- **Proper nouns.** Objects in the product _are not_ considered proper nouns e.g. dashboards, charts, saved queries etc. Proprietary feature names eg. SQL Lab, Preset Manager _are_ considered proper nouns
+- **Acronyms** (e.g. CSS, HTML)
+- When referring to **UI labels that are themselves capitalized** from sentence case (e.g. page titles - Dashboards page, Charts page, Saved queries page, etc.)
+- User input that is reflected in the UI. E.g. a user-named a dashboard tab
-**Sentence case vs. Title case:** Title case: "A Dog Takes a Walk in Paris" Sentence case: "A dog takes a walk in Paris"
+**Sentence case vs. Title case:**
+- Title case: "A Dog Takes a Walk in Paris"
+- Sentence case: "A dog takes a walk in Paris"
**Why sentence case?**
-* It's generally accepted as the quickest to read
-* It's the easiest form to distinguish between common and proper nouns
+- It's generally accepted as the quickest to read
+- It's the easiest form to distinguish between common and proper nouns
### How to refer to UI elements
@@ -52,21 +54,38 @@ When writing about a UI element, use the same capitalization as used in the UI.
For example, if an input field is labeled "Name" then you refer to this as the "Name input field". Similarly, if a button has the label "Save" in it, then it is correct to refer to the "Save button".
-Where a product page is titled "Settings", you refer to this in writing as follows: "Edit your personal information on the Settings page".
+Where a product page is titled "Settings", you refer to this in writing as follows:
+"Edit your personal information on the Settings page".
Often a product page will have the same title as the objects it contains. In this case, refer to the page as it appears in the UI, and the objects as common nouns:
-* Upload a dashboard on the Dashboards page
-* Go to Dashboards
-* View dashboard
-* View all dashboards
-* Upload CSS templates on the CSS templates page
-* Queries that you save will appear on the Saved queries page
-* Create custom queries in SQL Lab then create dashboards
+- Upload a dashboard on the Dashboards page
+- Go to Dashboards
+- View dashboard
+- View all dashboards
+- Upload CSS templates on the CSS templates page
+- Queries that you save will appear on the Saved queries page
+- Create custom queries in SQL Lab then create dashboards
-### **Exceptions to sentence case:**
+### Exceptions to sentence case
-1. Acronyms and abbreviations. Examples: URL, CSV, XML
+1. **Acronyms and abbreviations.**
+ Examples: URL, CSV, XML, CSS, SQL, SSH, URI, NaN, CRON, CC, BCC
+
+2. **Proper nouns and brand names.**
+ Examples: Apache, Superset, AntD JavaScript, GeoJSON, Slack, Google Sheets, SQLAlchemy
+
+3. **Technical terms derived from proper nouns.**
+ Examples: Jinja, Gaussian, European (as in European time zone)
+
+4. **Key names.** Capitalize button labels and UI elements as they appear in the product UI.
+ Examples: Shift (as in the keyboard button), Enter key
+
+5. **Named queries or specific labeled items.**
+ Examples: Query A, Query B
+
+6. **Database names.** Always capitalize names of database engines and connectors.
+ Examples: Presto, Trino, Drill, Hive, Google Sheets
## Button Design Guidelines
@@ -98,6 +117,32 @@ Primary buttons have a fourth style: dropdown.
| Tertiary | For less prominent actions; can be used in isolation or paired with a primary button |
| Destructive | For actions that could have destructive effects on the user's data |
+### Format
+
+#### Anatomy
+
+Button text is centered using the Label style. Icons appear left of text when combined. If no text label exists, an icon must indicate the button's function.
+
+#### Button size
+
+- Default dimensions: 160px width Ă— 32px height
+- Text: 11px, Inter Medium, all caps
+- Corners: 4px border radius
+- Minimum padding: 8px around text
+- Width can decrease if space is limited, but maintain minimum padding
+
+#### Button groups
+
+- Group related buttons to establish visual hierarchy
+- Avoid overwhelming users with too many actions
+- Limit calls to action; use tertiary/ghost buttons for layouts with 3+ actions
+- Maintain consistent styles within groups when possible
+- Space buttons 8px apart vertically or horizontally
+
+#### Content guidelines
+
+Button labels should be clear and predictable. Use the "\{verb\} + \{noun\}" format, except for common actions like "Done," "Close," "Cancel," "Add," or "Delete." This formula provides necessary context and aids translation, though compact UIs or localization needs may warrant exceptions.
+
## Error Message Design Guidelines
### Definition
@@ -128,10 +173,10 @@ In all cases, encountering errors increases user friction and frustration while
Select one pattern per error (e.g. do not implement an inline and banner pattern for the same error).
-When the error... | Use...
----------------- | ------
-Is directly related to a UI control | Inline error
-Is not directly related to a UI control | Banner error
+| When the error... | Use... |
+|------------------|--------|
+| Is directly related to a UI control | Inline error |
+| Is not directly related to a UI control | Banner error |
#### Inline
@@ -146,3 +191,45 @@ Use the `LabeledErrorBoundInput` component for this error pattern.
##### Implementation details
- Where and when relevant, scroll the screen to the UI control with the error
+- When multiple inline errors are present, scroll to the topmost error
+
+#### Banner
+
+Banner errors are used when the source of the error is not directly related to a UI control (text input, selector, etc.) such as a technical failure or a loading problem.
+
+##### Anatomy
+
+Use the `ErrorAlert` component for this error pattern.
+
+1. **Headline** (optional): 1-2 word summary of the error
+2. **Message**: What went wrong and what users should do next
+3. **Expand option** (optional): "See more"/"See less"
+4. **Details** (optional): Additional helpful context
+5. **Modal** (optional): For spatial constraints using `ToastType.DANGER`
+
+##### Implementation details
+
+- Place the banner near the content area most relevant to the error
+- For chart errors in Explore, use the chart area
+- For modal errors, use the modal footer
+- For app-wide errors, use the top of the screen
+
+### Content guidelines
+
+Effective error messages communicate:
+
+1. What went wrong
+2. What users should do next
+
+Error messages should be:
+
+- Clear and accurate, leaving no room for misinterpretation
+- Short and concise
+- Understandable to non-technical users
+- Non-blaming and avoiding negative language
+
+**Example:**
+
+❌ "Cannot delete a datasource that has slices attached to it."
+
+âś… "Please delete all charts using this dataset before deleting the dataset."
diff --git a/docs/developer_portal/guidelines/frontend-style-guidelines.md b/docs/developer_portal/guidelines/frontend-style-guidelines.md
index 47104db221..b27cbafb5e 100644
--- a/docs/developer_portal/guidelines/frontend-style-guidelines.md
+++ b/docs/developer_portal/guidelines/frontend-style-guidelines.md
@@ -1,6 +1,6 @@
---
-title: Frontend Style Guidelines
-sidebar_position: 2
+title: Overview
+sidebar_position: 1
---
<!--
@@ -26,19 +26,25 @@ under the License.
This is a list of statements that describe how we do frontend development in Superset. While they might not be 100% true for all files in the repo, they represent the gold standard we strive towards for frontend quality and style.
-* We develop using TypeScript.
-* We use React for building components, and Redux to manage app/global state.
- * See: [Component Style Guidelines and Best Practices](./frontend/component-style-guidelines)
-* We prefer functional components to class components and use hooks for local component state.
-* We use [Ant Design](https://ant.design/) components from our component library whenever possible, only building our own custom components when it's required.
-* We use [@emotion](https://emotion.sh/docs/introduction) to provide styling for our components, co-locating styling within component files.
- * See: [Emotion Styling Guidelines and Best Practices](./frontend/emotion-styling-guidelines)
-* We use Jest for unit tests, React Testing Library for component tests, and Cypress for end-to-end tests.
- * See: [Testing Guidelines and Best Practices](./frontend/testing-guidelines)
-* We add tests for every new component or file added to the frontend.
-* We organize our repo so similar files live near each other, and tests are co-located with the files they test.
-* We prefer small, easily testable files and components.
-* We use ESLint and Prettier to automatically fix lint errors and format the code.
- * We do not debate code formatting style in PRs, instead relying on automated tooling to enforce it.
- * If there's not a linting rule, we don't have a rule!
-* We use [React Storybook](https://storybook.js.org/) and [Applitools](https://applitools.com/) to help preview/test and stabilize our components
+- We develop using TypeScript.
+ - See: [SIP-36](https://github.com/apache/superset/issues/9101)
+- We use React for building components, and Redux to manage app/global state.
+ - See: [Component Style Guidelines and Best Practices](./frontend/component-style-guidelines)
+- We prefer functional components to class components and use hooks for local component state.
+- We use [Ant Design](https://ant.design/) components from our component library whenever possible, only building our own custom components when it's required.
+ - See: [SIP-48](https://github.com/apache/superset/issues/11283)
+- We use [@emotion](https://emotion.sh/docs/introduction) to provide styling for our components, co-locating styling within component files.
+ - See: [SIP-37](https://github.com/apache/superset/issues/9145)
+ - See: [Emotion Styling Guidelines and Best Practices](./frontend/emotion-styling-guidelines)
+- We use Jest for unit tests, React Testing Library for component tests, and Cypress for end-to-end tests.
+ - See: [SIP-56](https://github.com/apache/superset/issues/11830)
+ - See: [Testing Guidelines and Best Practices](../testing/testing-guidelines)
+- We add tests for every new component or file added to the frontend.
+- We organize our repo so similar files live near each other, and tests are co-located with the files they test.
+ - See: [SIP-61](https://github.com/apache/superset/issues/12098)
+- We prefer small, easily testable files and components.
+- We use ESLint and Prettier to automatically fix lint errors and format the code.
+ - We do not debate code formatting style in PRs, instead relying on automated tooling to enforce it.
+ - If there's not a linting rule, we don't have a rule!
+- We use [React Storybook](https://storybook.js.org/) and [Applitools](https://applitools.com/) to help preview/test and stabilize our components
+ - A public Storybook with components from the `master` branch is available [here](https://apache-superset.github.io/superset-ui/?path=/story/*)
diff --git a/docs/developer_portal/guidelines/frontend/component-style-guidelines.md b/docs/developer_portal/guidelines/frontend/component-style-guidelines.md
index 95a88c9559..faf1b179e7 100644
--- a/docs/developer_portal/guidelines/frontend/component-style-guidelines.md
+++ b/docs/developer_portal/guidelines/frontend/component-style-guidelines.md
@@ -1,6 +1,6 @@
---
title: Component Style Guidelines and Best Practices
-sidebar_position: 1
+sidebar_position: 2
---
<!--
@@ -35,7 +35,7 @@ This guide is intended primarily for reusable components. Whenever possible, all
- All components should be made to be reusable whenever possible
- All components should follow the structure and best practices as detailed below
-## Directory and component structure
+### Directory and component structure
```
superset-frontend/src/components
@@ -51,208 +51,142 @@ superset-frontend/src/components
**Component directory name:** Use `PascalCase` for the component directory name
-**Storybook:** Components should come with a storybook file whenever applicable, with the following naming convention `{ComponentName}.stories.tsx`. More details about Storybook below
+**Storybook:** Components should come with a storybook file whenever applicable, with the following naming convention `\{ComponentName\}.stories.tsx`. More details about Storybook below
-**Unit and end-to-end tests:** All components should come with unit tests using Jest and React Testing Library. The file name should follow this naming convention `{ComponentName}.test.tsx.` Read the [Testing Guidelines and Best Practices](./testing-guidelines) for more details about tests
+**Unit and end-to-end tests:** All components should come with unit tests using Jest and React Testing Library. The file name should follow this naming convention `\{ComponentName\}.test.tsx`. Read the [Testing Guidelines and Best Practices](../../testing/testing-guidelines) for more details
-## Component Development Best Practices
+**Reference naming:** Use `PascalCase` for React components and `camelCase` for component instances
-### Use TypeScript
+**BAD:**
+```jsx
+import mainNav from './MainNav';
+```
-All new components should be written in TypeScript. This helps catch errors early and provides better development experience with IDE support.
+**GOOD:**
+```jsx
+import MainNav from './MainNav';
+```
-```tsx
-interface ComponentProps {
- title: string;
- isVisible?: boolean;
- onClose?: () => void;
-}
+**BAD:**
+```jsx
+const NavItem = <MainNav />;
+```
-export const MyComponent: React.FC<ComponentProps> = ({
- title,
- isVisible = true,
- onClose
-}) => {
- // Component implementation
-};
+**GOOD:**
+```jsx
+const navItem = <MainNav />;
```
-### Prefer Functional Components
+**Component naming:** Use the file name as the component name
-Use functional components with hooks instead of class components:
+**BAD:**
+```jsx
+import MainNav from './MainNav/index';
+```
-```tsx
-// âś… Good - Functional component with hooks
-export const MyComponent: React.FC<Props> = ({ data }) => {
- const [loading, setLoading] = useState(false);
+**GOOD:**
+```jsx
+import MainNav from './MainNav';
+```
- useEffect(() => {
- // Effect logic
- }, []);
+**Props naming:** Do not use DOM related props for different purposes
- return <div>{/* Component JSX */}</div>;
-};
+**BAD:**
+```jsx
+<MainNav style="big" />
+```
-// ❌ Avoid - Class component
-class MyComponent extends React.Component {
- // Class implementation
-}
+**GOOD:**
+```jsx
+<MainNav variant="big" />
```
-### Follow Ant Design Patterns
+**Importing dependencies:** Only import what you need
-Extend Ant Design components rather than building from scratch:
+**BAD:**
+```jsx
+import * as React from "react";
+```
-```tsx
-import { Button } from 'antd';
-import styled from '@emotion/styled';
+**GOOD:**
+```jsx
+import React, { useState } from "react";
+```
+
+**Default VS named exports:** As recommended by [TypeScript](https://www.typescriptlang.org/docs/handbook/modules.html), "If a module's primary purpose is to house one specific export, then you should consider exporting it as a default export. This makes both importing and actually using the import a little easier". If you're exporting multiple objects, use named exports instead.
-const StyledButton = styled(Button)`
- // Custom styling using emotion
-`;
+_As a default export_
+```jsx
+import MainNav from './MainNav';
```
-### Reusability and Props Design
+_As a named export_
+```jsx
+import { MainNav, SecondaryNav } from './Navbars';
+```
-Design components with reusability in mind:
+**ARIA roles:** Always make sure you are writing accessible components by using the official [ARIA roles](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques)
-```tsx
-interface ButtonProps {
- variant?: 'primary' | 'secondary' | 'tertiary';
- size?: 'small' | 'medium' | 'large';
- loading?: boolean;
- disabled?: boolean;
- children: React.ReactNode;
- onClick?: () => void;
-}
+## Use TypeScript
-export const CustomButton: React.FC<ButtonProps> = ({
- variant = 'primary',
- size = 'medium',
- ...props
-}) => {
- // Implementation
-};
-```
+All components should be written in TypeScript and their extensions should be `.ts` or `.tsx`
-## Testing Components
+### type vs interface
-Every component should include comprehensive tests:
+Validate all props with the correct types. This replaces the need for a run-time validation as provided by the prop-types library.
```tsx
-// MyComponent.test.tsx
-import { render, screen, fireEvent } from '@testing-library/react';
-import { MyComponent } from './MyComponent';
-
-test('renders component with title', () => {
- render(<MyComponent title="Test Title" />);
- expect(screen.getByText('Test Title')).toBeInTheDocument();
-});
-
-test('calls onClose when close button is clicked', () => {
- const mockOnClose = jest.fn();
- render(<MyComponent title="Test" onClose={mockOnClose} />);
-
- fireEvent.click(screen.getByRole('button', { name: /close/i }));
- expect(mockOnClose).toHaveBeenCalledTimes(1);
-});
+type HeadingProps = {
+ param: string;
+}
+
+export default function Heading({ children }: HeadingProps) {
+ return <h2>{children}</h2>
+}
```
-## Storybook Stories
+Use `type` for your component props and state. Use `interface` when you want to enable _declaration merging_.
+
+### Define default values for non-required props
-Create stories for visual testing and documentation:
+In order to improve the readability of your code and reduce assumptions, always add default values for non required props, when applicable, for example:
```tsx
-// MyComponent.stories.tsx
-import type { Meta, StoryObj } from '@storybook/react';
-import { MyComponent } from './MyComponent';
-
-const meta: Meta<typeof MyComponent> = {
- title: 'Components/MyComponent',
- component: MyComponent,
- parameters: {
- layout: 'centered',
- },
- tags: ['autodocs'],
-};
-
-export default meta;
-type Story = StoryObj<typeof meta>;
-
-export const Default: Story = {
- args: {
- title: 'Default Component',
- isVisible: true,
- },
-};
-
-export const Hidden: Story = {
- args: {
- title: 'Hidden Component',
- isVisible: false,
- },
-};
+const applyDiscount = (price: number, discount = 0.05) => price * (1 - discount);
```
-## Performance Considerations
+## Functional components and Hooks
-### Use React.memo for Expensive Components
+We prefer functional components and the usage of hooks over class components.
-```tsx
-import React, { memo } from 'react';
+### useState
+
+Always explicitly declare the type unless the type can easily be assumed by the declaration.
-export const ExpensiveComponent = memo<Props>(({ data }) => {
- // Expensive rendering logic
- return <div>{/* Component content */}</div>;
-});
+```tsx
+const [customer, setCustomer] = useState<ICustomer | null>(null);
```
-### Optimize Re-renders
+### useReducer
-Use `useCallback` and `useMemo` appropriately:
+Always prefer `useReducer` over `useState` when your state has complex logics.
-```tsx
-export const OptimizedComponent: React.FC<Props> = ({ items, onSelect }) => {
- const expensiveValue = useMemo(() => {
- return items.reduce((acc, item) => acc + item.value, 0);
- }, [items]);
+### useMemo and useCallback
- const handleSelect = useCallback((id: string) => {
- onSelect(id);
- }, [onSelect]);
+Always memoize when your components take functions or complex objects as props to avoid unnecessary rerenders.
- return <div>{/* Component content */}</div>;
-};
-```
+### Custom hooks
-## Accessibility
+All custom hooks should be located in the directory `/src/hooks`. Before creating a new custom hook, make sure that is not available in the existing custom hooks.
-Ensure components are accessible:
+## Storybook
-```tsx
-export const AccessibleButton: React.FC<Props> = ({ children, onClick }) => {
- return (
- <button
- type="button"
- aria-label="Descriptive label"
- onClick={onClick}
- >
- {children}
- </button>
- );
-};
-```
+Each component should come with its dedicated storybook file.
-## Error Boundaries
+**One component per story:** Each storybook file should only contain one component unless substantially different variants are required
-For components that might fail, consider error boundaries:
+**Component variants:** If the component behavior is substantially different when certain props are used, it is best to separate the story into different types. See the `superset-frontend/src/components/Select/Select.stories.tsx` as an example.
-```tsx
-export const SafeComponent: React.FC<Props> = ({ children }) => {
- return (
- <ErrorBoundary fallback={<div>Something went wrong</div>}>
- {children}
- </ErrorBoundary>
- );
-};
-```
+**Isolated state:** The storybook should show how the component works in an isolated state and with as few dependencies as possible
+
+**Use args:** It should be possible to test the component with every variant of the available props. We recommend using [args](https://storybook.js.org/docs/react/writing-stories/args)
diff --git a/docs/developer_portal/guidelines/frontend/emotion-styling-guidelines.md b/docs/developer_portal/guidelines/frontend/emotion-styling-guidelines.md
index 77d6687f30..7a2352aeb5 100644
--- a/docs/developer_portal/guidelines/frontend/emotion-styling-guidelines.md
+++ b/docs/developer_portal/guidelines/frontend/emotion-styling-guidelines.md
@@ -1,6 +1,6 @@
---
title: Emotion Styling Guidelines and Best Practices
-sidebar_position: 2
+sidebar_position: 3
---
<!--
@@ -33,314 +33,245 @@ under the License.
- **DO** use `css` when you want to amend/merge sets of styles compositionally
- **DO** use `css` when you're making a small, or single-use set of styles for a component
- **DO** move your style definitions from direct usage in the `css` prop to an external variable when they get long
-- **DO** prefer tagged template literals (`css={css\`...\`}`) over style objects wherever possible for maximum style portability/consistency
-- **DO** use `useTheme` to get theme variables
+- **DO** prefer tagged template literals (`css={css`...`}`) over style objects wherever possible for maximum style portability/consistency (note: typescript support may be diminished, but IDE plugins like [this](https://marketplace.visualstudio.com/items?itemName=jpoissonnier.vscode-styled-components) make life easy)
+- **DO** use `useTheme` to get theme variables. `withTheme` should be only used for wrapping legacy Class-based components.
### DON'T do these things:
- **DON'T** use `styled` for small, single-use style tweaks that would be easier to read/review if they were inline
-- **DON'T** export incomplete Ant Design components
+- **DON'T** export incomplete AntD components (make sure all their compound components are exported)
-## When to use `css` or `styled`
-
-### Use `css` for:
-
-1. **Small, single-use styles**
-```tsx
-import { css } from '@emotion/react';
-
-const MyComponent = () => (
- <div
- css={css`
- margin: 8px;
- padding: 16px;
- `}
- >
- Content
- </div>
-);
-```
-
-2. **Composing styles**
-```tsx
-const baseStyles = css`
- padding: 16px;
- border-radius: 4px;
-`;
+## Emotion Tips and Strategies
-const primaryStyles = css`
- ${baseStyles}
- background-color: blue;
- color: white;
-`;
+The first thing to consider when adding styles to an element is how much you think a style might be reusable in other areas of Superset. Always err on the side of reusability here. Nobody wants to chase styling inconsistencies, or try to debug little endless overrides scattered around the codebase. The more we can consolidate, the less will have to be figured out by those who follow. Reduce, reuse, recycle.
-const secondaryStyles = css`
- ${baseStyles}
- background-color: gray;
- color: black;
-`;
-```
-
-3. **Conditional styling**
-```tsx
-const MyComponent = ({ isActive }: { isActive: boolean }) => (
- <div
- css={[
- baseStyles,
- isActive && activeStyles,
- ]}
- >
- Content
- </div>
-);
-```
+## When to use `css` or `styled`
-### Use `styled` for:
+In short, either works for just about any use case! And you'll see them used somewhat interchangeably in the existing codebase. But we need a way to weigh it when we encounter the choice, so here's one way to think about it:
-1. **Reusable components**
-```tsx
-import styled from '@emotion/styled';
+A good use of `styled` syntax if you want to re-use a styled component. In other words, if you wanted to export flavors of a component for use, like so:
-const StyledButton = styled.button`
- padding: 12px 24px;
- border: none;
- border-radius: 4px;
- background-color: ${({ theme }) => theme.colors.primary};
- color: white;
+```jsx
+const StatusThing = styled.div`
+ padding: 10px;
+ border-radius: 10px;
+`;
- &:hover {
- background-color: ${({ theme }) => theme.colors.primaryDark};
+export const InfoThing = styled(StatusThing)`
+ background: blue;
+ &::before {
+ content: "ℹ️";
}
`;
-```
-
-2. **Components with complex nested selectors**
-```tsx
-const StyledCard = styled.div`
- padding: 16px;
- border: 1px solid ${({ theme }) => theme.colors.border};
- .card-header {
- font-weight: bold;
- margin-bottom: 8px;
+export const WarningThing = styled(StatusThing)`
+ background: orange;
+ &::before {
+ content: "⚠️";
}
+`;
- .card-content {
- color: ${({ theme }) => theme.colors.text};
-
- p {
- margin-bottom: 12px;
- }
+export const TerribleThing = styled(StatusThing)`
+ background: red;
+ &::before {
+ content: "🔥";
}
`;
```
-3. **Extending Ant Design components**
-```tsx
-import { Button } from 'antd';
-import styled from '@emotion/styled';
-
-const CustomButton = styled(Button)`
- border-radius: 8px;
- font-weight: 600;
+You can also use `styled` when you're building a bigger component, and just want to have some custom bits for internal use in your JSX. For example:
- &.ant-btn-primary {
- background: linear-gradient(45deg, #1890ff, #722ed1);
- }
+```jsx
+const SeparatorOnlyUsedInThisComponent = styled.hr`
+ height: 12px;
+ border: 0;
+ box-shadow: inset 0 12px 12px -12px rgba(0, 0, 0, 0.5);
`;
+
+function SuperComplicatedComponent(props) {
+ return (
+ <>
+ Daily standup for {user.name}!
+ <SeparatorOnlyUsedInThisComponent />
+ <h2>Yesterday:</h2>
+ // spit out a list of accomplishments
+ <SeparatorOnlyUsedInThisComponent />
+ <h2>Today:</h2>
+ // spit out a list of plans
+ <SeparatorOnlyUsedInThisComponent />
+ <h2>Tomorrow:</h2>
+ // spit out a list of goals
+ </>
+ );
+}
```
-## Using Theme Variables
+The `css` prop, in reality, shares all the same styling capabilities as `styled` but it does have some particular use cases that jump out as sensible. For example, if you just want to style one element in your component, you could add the styles inline like so:
-Always use theme variables for consistent styling:
+```jsx
+function SomeFanciness(props) {
+ return (
+ <>
+ Here's an awesome report card for {user.name}!
+ <div
+ css={css`
+ box-shadow: 5px 5px 10px #ccc;
+ border-radius: 10px;
+ `}
+ >
+ <h2>Yesterday:</h2>
+ // ...some stuff
+ <h2>Today:</h2>
+ // ...some stuff
+ <h2>Tomorrow:</h2>
+ // ...some stuff
+ </div>
+ </>
+ );
+}
+```
-```tsx
-import { useTheme } from '@emotion/react';
+You can also define the styles as a variable, external to your JSX. This is handy if the styles get long and you just want it out of the way. This is also handy if you want to apply the same styles to disparate element types, kind of like you might use a CSS class on varied elements. Here's a trumped up example:
-const MyComponent = () => {
- const theme = useTheme();
+```jsx
+function FakeGlobalNav(props) {
+ const menuItemStyles = css`
+ display: block;
+ border-bottom: 1px solid cadetblue;
+ font-family: "Comic Sans", cursive;
+ `;
return (
- <div
- css={css`
- background-color: ${theme.colors.grayscale.light5};
- color: ${theme.colors.grayscale.dark2};
- padding: ${theme.gridUnit * 4}px;
- border-radius: ${theme.borderRadius}px;
- `}
- >
- Content
- </div>
+ <Nav>
+ <a css={menuItemStyles} href="#">One link</a>
+ <Link css={menuItemStyles} to={url}>Another link</Link>
+ <div css={menuItemStyles} onClick={() => alert('clicked')}>Another link</div>
+ </Nav>
);
-};
+}
```
-## Common Patterns
+## CSS tips and tricks
-### Responsive Design
-```tsx
-const ResponsiveContainer = styled.div`
- display: flex;
- flex-direction: column;
+### `css` lets you write actual CSS
- ${({ theme }) => theme.breakpoints.up('md')} {
- flex-direction: row;
- }
-`;
+By default the `css` prop uses the object syntax with JS style definitions, like so:
+
+```jsx
+<div css={{
+ borderRadius: 10,
+ marginTop: 10,
+ backgroundColor: '#00FF00'
+}}>Howdy</div>
```
-### Animation
-```tsx
-const FadeInComponent = styled.div`
- opacity: 0;
- transition: opacity 0.3s ease-in-out;
+But you can use the `css` interpolator as well to get away from icky JS styling syntax. Doesn't this look cleaner?
- &.visible {
- opacity: 1;
- }
-`;
+```jsx
+<div css={css`
+ border-radius: 10px;
+ margin-top: 10px;
+ background-color: #00FF00;
+`}>Howdy</div>
```
-### Conditional Props
-```tsx
-interface StyledDivProps {
- isHighlighted?: boolean;
- size?: 'small' | 'medium' | 'large';
+You might say "whatever… I can read and write JS syntax just fine." Well, that's great. But… let's say you're migrating in some of our legacy LESS styles… now it's copy/paste! Or if you want to migrate to or from `styled` syntax… also copy/paste!
+
+### You can combine `css` definitions with array syntax
+
+You can use multiple groupings of styles with the `css` interpolator, and combine/override them in array syntax, like so:
+
+```jsx
+function AnotherSillyExample(props) {
+ const shadowedCard = css`
+ box-shadow: 2px 2px 4px #999;
+ padding: 4px;
+ `;
+ const infoCard = css`
+ background-color: #33f;
+ border-radius: 4px;
+ `;
+ const overrideInfoCard = css`
+ background-color: #f33;
+ `;
+ return (
+ <div className="App">
+ Combining two classes:
+ <div css={[shadowedCard, infoCard]}>Hello</div>
+ Combining again, but now with overrides:
+ <div css={[shadowedCard, infoCard, overrideInfoCard]}>Hello</div>
+ </div>
+ );
}
-
-const StyledDiv = styled.div<StyledDivProps>`
- padding: 16px;
- background-color: ${({ isHighlighted, theme }) =>
- isHighlighted ? theme.colors.primary : theme.colors.grayscale.light5};
-
- ${({ size }) => {
- switch (size) {
- case 'small':
- return css`font-size: 12px;`;
- case 'large':
- return css`font-size: 18px;`;
- default:
- return css`font-size: 14px;`;
- }
- }}
-`;
```
-## Best Practices
+### Style variations with props
-### 1. Use Semantic CSS Properties
-```tsx
-// âś… Good - semantic property names
-const Header = styled.h1`
- font-size: ${({ theme }) => theme.typography.sizes.xl};
- margin-bottom: ${({ theme }) => theme.gridUnit * 4}px;
-`;
+You can give any component a custom prop, and reference that prop in your component styles, effectively using the prop to turn on a "flavor" of that component.
-// ❌ Avoid - magic numbers
-const Header = styled.h1`
- font-size: 24px;
- margin-bottom: 16px;
+For example, let's make a styled component that acts as a card. Of course, this could be done with any AntD component, or any component at all. But we'll do this with a humble `div` to illustrate the point:
+
+```jsx
+const SuperCard = styled.div`
+ ${({ theme, cutout }) => `
+ padding: ${theme.gridUnit * 2}px;
+ border-radius: ${theme.borderRadius}px;
+ box-shadow: 10px 5px 10px #ccc ${cutout && 'inset'};
+ border: 1px solid ${cutout ? 'transparent' : theme.colors.secondary.light3};
+ `}
`;
```
-### 2. Group Related Styles
-```tsx
-// âś… Good - grouped styles
-const Card = styled.div`
- /* Layout */
- display: flex;
- flex-direction: column;
- padding: ${({ theme }) => theme.gridUnit * 4}px;
-
- /* Appearance */
- background-color: ${({ theme }) => theme.colors.grayscale.light5};
- border: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
- border-radius: ${({ theme }) => theme.borderRadius}px;
+Then just use the component as `<SuperCard>Some content</SuperCard>` or with the (potentially dynamic) prop: `<SuperCard cutout>Some content</SuperCard>`
+
+## Styled component tips
+
+### No need to use `theme` the hard way
+
+It's very tempting (and commonly done) to use the `theme` prop inline in the template literal like so:
- /* Typography */
- font-family: ${({ theme }) => theme.typography.families.sansSerif};
- color: ${({ theme }) => theme.colors.grayscale.dark2};
+```jsx
+const SomeStyledThing = styled.div`
+ padding: ${({ theme }) => theme.gridUnit * 2}px;
+ border-radius: ${({ theme }) => theme.borderRadius}px;
+ border: 1px solid ${({ theme }) => theme.colors.secondary.light3};
`;
```
-### 3. Extract Complex Styles
-```tsx
-// âś… Good - extract complex styles to variables
-const complexGradient = css`
- background: linear-gradient(
- 135deg,
- ${({ theme }) => theme.colors.primary} 0%,
- ${({ theme }) => theme.colors.secondary} 100%
- );
-`;
+Instead, you can make things a little easier to read/type by writing it like so:
-const GradientButton = styled.button`
- ${complexGradient}
- padding: 12px 24px;
- border: none;
- color: white;
+```jsx
+const SomeStyledThing = styled.div`
+ ${({ theme }) => `
+ padding: ${theme.gridUnit * 2}px;
+ border-radius: ${theme.borderRadius}px;
+ border: 1px solid ${theme.colors.secondary.light3};
+ `}
`;
```
-### 4. Avoid Deep Nesting
-```tsx
-// âś… Good - shallow nesting
-const Navigation = styled.nav`
- .nav-item {
- padding: 8px 16px;
- }
+## Extend an AntD component with custom styling
- .nav-link {
- color: ${({ theme }) => theme.colors.text};
- text-decoration: none;
- }
-`;
+As mentioned, you want to keep your styling as close to the root of your component system as possible, to minimize repetitive styling/overrides, and err on the side of reusability. In some cases, that means you'll want to globally tweak one of our core components to match our design system. In Superset, that's Ant Design (AntD).
-// ❌ Avoid - deep nesting
-const Navigation = styled.nav`
- ul {
- li {
- a {
- span {
- color: blue; /* Too nested */
- }
- }
- }
- }
-`;
-```
+AntD uses a cool trick called compound components. For example, the `Menu` component also lets you use `Menu.Item`, `Menu.SubMenu`, `Menu.ItemGroup`, and `Menu.Divider`.
-## Performance Tips
+### The `Object.assign` trick
-### 1. Avoid Inline Functions in CSS
-```tsx
-// âś… Good - external function
-const getBackgroundColor = (isActive: boolean, theme: any) =>
- isActive ? theme.colors.primary : theme.colors.secondary;
+Let's say you want to override an AntD component called `Foo`, and have `Foo.Bar` display some custom CSS for the `Bar` compound component. You can do it effectively like so:
-const Button = styled.button<{ isActive: boolean }>`
- background-color: ${({ isActive, theme }) => getBackgroundColor(isActive, theme)};
-`;
+```jsx
+import {
+ Foo as AntdFoo,
+} from 'antd';
-// ❌ Avoid - inline function (creates new function on each render)
-const Button = styled.button<{ isActive: boolean }>`
- background-color: ${({ isActive, theme }) =>
- isActive ? theme.colors.primary : theme.colors.secondary};
+export const StyledBar = styled(AntdFoo.Bar)`
+ border-radius: ${({ theme }) => theme.borderRadius}px;
`;
-```
-### 2. Use CSS Objects for Dynamic Styles
-```tsx
-// For highly dynamic styles, consider CSS objects
-const dynamicStyles = (props: Props) => ({
- backgroundColor: props.color,
- fontSize: `${props.size}px`,
- // ... other dynamic properties
+export const Foo = Object.assign(AntdFoo, {
+ Bar: StyledBar,
});
-
-const DynamicComponent = (props: Props) => (
- <div css={dynamicStyles(props)}>
- Content
- </div>
-);
```
+
+You can then import this customized `Foo` and use `Foo.Bar` as expected. You should probably save your creation in `src/components` for maximum reusability, and add a Storybook entry so future engineers can view your creation, and designers can better understand how it fits the Superset Design System.
diff --git a/docs/developer_portal/guidelines/frontend/testing-guidelines.md b/docs/developer_portal/guidelines/frontend/testing-guidelines.md
deleted file mode 100644
index 0604af4eac..0000000000
--- a/docs/developer_portal/guidelines/frontend/testing-guidelines.md
+++ /dev/null
@@ -1,297 +0,0 @@
----
-title: Testing Guidelines and Best Practices
-sidebar_position: 3
----
-
-<!--
-Licensed to the Apache Software Foundation (ASF) under one
-or more contributor license agreements. See the NOTICE file
-distributed with this work for additional information
-regarding copyright ownership. The ASF licenses this file
-to you under the Apache License, Version 2.0 (the
-"License"); you may not use this file except in compliance
-with the License. You may obtain a copy of the License at
-
- http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing,
-software distributed under the License is distributed on an
-"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-KIND, either express or implied. See the License for the
-specific language governing permissions and limitations
-under the License.
--->
-
-# Testing Guidelines and Best Practices
-
-We feel that tests are an important part of a feature and not an additional or optional effort. That's why we colocate test files with functionality and sometimes write tests upfront to help validate requirements and shape the API of our components. Every new component or file added should have an associated test file with the `.test` extension.
-
-We use Jest, React Testing Library (RTL), and Cypress to write our unit, integration, and end-to-end tests. For each type, we have a set of best practices/tips described below:
-
-## Jest
-
-### Write simple, standalone tests
-
-The importance of simplicity is often overlooked in test cases. Clear, dumb code should always be preferred over complex ones. The test cases should be pretty much standalone and should not involve any external logic if not absolutely necessary. That's because you want the corpus of the tests to be easy to read and understandable at first sight.
-
-### Avoid nesting when you're testing
-
-Avoid the use of `describe` blocks in favor of inlined tests. If your tests start to grow and you feel the need to group tests, prefer to break them into multiple test files. Check this awesome [article](https://kentcdodds.com/blog/avoid-nesting-when-youre-testing) written by [Kent C. Dodds](https://kentcdodds.com/) about this topic.
-
-### No warnings!
-
-Your tests shouldn't trigger warnings. This is really common when testing async functionality. It's really difficult to read test results when we have a bunch of warnings.
-
-## React Testing Library (RTL)
-
-### Keep accessibility in mind when writing your tests
-
-One of the most important points of RTL is accessibility and this is also a very important point for us. We should try our best to follow the RTL [Priority](https://testing-library.com/docs/queries/about/#priority) when querying for elements in our tests. `getByTestId` is not viewable by the user and should only be used when the element isn't accessible in any other way.
-
-### Don't use `act` unnecessarily
-
-`render` and `fireEvent` are already wrapped in `act`, so wrapping them in `act` again is a common mistake. Some solutions to the warnings related to async testing can be found in the RTL docs.
-
-## Example Test Structure
-
-```tsx
-// MyComponent.test.tsx
-import { render, screen, fireEvent, waitFor } from '@testing-library/react';
-import userEvent from '@testing-library/user-event';
-import { MyComponent } from './MyComponent';
-
-// âś… Good - Simple, standalone test
-test('renders loading state initially', () => {
- render(<MyComponent />);
- expect(screen.getByText('Loading...')).toBeInTheDocument();
-});
-
-// âś… Good - Tests user interaction
-test('calls onSubmit when form is submitted', async () => {
- const user = userEvent.setup();
- const mockOnSubmit = jest.fn();
-
- render(<MyComponent onSubmit={mockOnSubmit} />);
-
- await user.type(screen.getByLabelText('Username'), 'testuser');
- await user.click(screen.getByRole('button', { name: 'Submit' }));
-
- expect(mockOnSubmit).toHaveBeenCalledWith({ username: 'testuser' });
-});
-
-// âś… Good - Tests async behavior
-test('displays error message when API call fails', async () => {
- const mockFetch = jest.fn().mockRejectedValue(new Error('API Error'));
- global.fetch = mockFetch;
-
- render(<MyComponent />);
-
- await waitFor(() => {
- expect(screen.getByText('Error: API Error')).toBeInTheDocument();
- });
-});
-```
-
-## Testing Best Practices
-
-### Use appropriate queries in priority order
-
-1. **Accessible to everyone**: `getByRole`, `getByLabelText`, `getByPlaceholderText`, `getByText`
-2. **Semantic queries**: `getByAltText`, `getByTitle`
-3. **Test IDs**: `getByTestId` (last resort)
-
-```tsx
-// âś… Good - using accessible queries
-test('user can submit form', () => {
- render(<LoginForm />);
-
- const usernameInput = screen.getByLabelText('Username');
- const passwordInput = screen.getByLabelText('Password');
- const submitButton = screen.getByRole('button', { name: 'Log in' });
-
- // Test implementation
-});
-
-// ❌ Avoid - using test IDs when better options exist
-test('user can submit form', () => {
- render(<LoginForm />);
-
- const usernameInput = screen.getByTestId('username-input');
- const passwordInput = screen.getByTestId('password-input');
- const submitButton = screen.getByTestId('submit-button');
-
- // Test implementation
-});
-```
-
-### Test behavior, not implementation details
-
-```tsx
-// âś… Good - tests what the user experiences
-test('shows success message after successful form submission', async () => {
- render(<ContactForm />);
-
- await userEvent.type(screen.getByLabelText('Email'), 'test@example.com');
- await userEvent.click(screen.getByRole('button', { name: 'Submit' }));
-
- await waitFor(() => {
- expect(screen.getByText('Message sent successfully!')).toBeInTheDocument();
- });
-});
-
-// ❌ Avoid - testing implementation details
-test('calls setState when form is submitted', () => {
- const component = shallow(<ContactForm />);
- const instance = component.instance();
- const spy = jest.spyOn(instance, 'setState');
-
- instance.handleSubmit();
- expect(spy).toHaveBeenCalled();
-});
-```
-
-### Mock external dependencies appropriately
-
-```tsx
-// Mock API calls
-jest.mock('../api/userService', () => ({
- getUser: jest.fn(),
- createUser: jest.fn(),
-}));
-
-// Mock components that aren't relevant to the test
-jest.mock('../Chart/Chart', () => {
- return function MockChart() {
- return <div data-testid="mock-chart">Chart Component</div>;
- };
-});
-```
-
-## Async Testing Patterns
-
-### Testing async operations
-
-```tsx
-test('loads and displays user data', async () => {
- const mockUser = { id: 1, name: 'John Doe' };
- const mockGetUser = jest.fn().mockResolvedValue(mockUser);
-
- render(<UserProfile getUserData={mockGetUser} />);
-
- // Wait for the async operation to complete
- await waitFor(() => {
- expect(screen.getByText('John Doe')).toBeInTheDocument();
- });
-
- expect(mockGetUser).toHaveBeenCalledTimes(1);
-});
-```
-
-### Testing loading states
-
-```tsx
-test('shows loading spinner while fetching data', async () => {
- const mockGetUser = jest.fn().mockImplementation(
- () => new Promise(resolve => setTimeout(resolve, 100))
- );
-
- render(<UserProfile getUserData={mockGetUser} />);
-
- expect(screen.getByText('Loading...')).toBeInTheDocument();
-
- await waitFor(() => {
- expect(screen.queryByText('Loading...')).not.toBeInTheDocument();
- });
-});
-```
-
-## Component-Specific Testing
-
-### Testing form components
-
-```tsx
-test('validates required fields', async () => {
- render(<RegistrationForm />);
-
- const submitButton = screen.getByRole('button', { name: 'Register' });
- await userEvent.click(submitButton);
-
- expect(screen.getByText('Username is required')).toBeInTheDocument();
- expect(screen.getByText('Email is required')).toBeInTheDocument();
-});
-```
-
-### Testing modals and overlays
-
-```tsx
-test('opens and closes modal', async () => {
- render(<ModalContainer />);
-
- const openButton = screen.getByRole('button', { name: 'Open Modal' });
- await userEvent.click(openButton);
-
- expect(screen.getByRole('dialog')).toBeInTheDocument();
-
- const closeButton = screen.getByRole('button', { name: 'Close' });
- await userEvent.click(closeButton);
-
- expect(screen.queryByRole('dialog')).not.toBeInTheDocument();
-});
-```
-
-### Testing with context providers
-
-```tsx
-const renderWithTheme = (component: React.ReactElement) => {
- return render(
- <ThemeProvider theme={mockTheme}>
- {component}
- </ThemeProvider>
- );
-};
-
-test('applies theme colors correctly', () => {
- renderWithTheme(<ThemedButton />);
-
- const button = screen.getByRole('button');
- expect(button).toHaveStyle({
- backgroundColor: mockTheme.colors.primary,
- });
-});
-```
-
-## Performance Testing
-
-### Testing with large datasets
-
-```tsx
-test('handles large lists efficiently', () => {
- const largeDataset = Array.from({ length: 10000 }, (_, i) => ({
- id: i,
- name: `Item ${i}`,
- }));
-
- const { container } = render(<VirtualizedList items={largeDataset} />);
-
- // Should only render visible items
- const renderedItems = container.querySelectorAll('[data-testid="list-item"]');
- expect(renderedItems.length).toBeLessThan(100);
-});
-```
-
-## Testing Accessibility
-
-```tsx
-test('is accessible to screen readers', () => {
- render(<AccessibleForm />);
-
- const form = screen.getByRole('form');
- const inputs = screen.getAllByRole('textbox');
-
- inputs.forEach(input => {
- expect(input).toHaveAttribute('aria-label');
- });
-
- expect(form).toHaveAttribute('aria-describedby');
-});
-```
diff --git a/docs/developer_portal/testing/testing-guidelines.md b/docs/developer_portal/testing/testing-guidelines.md
new file mode 100644
index 0000000000..5fb8424449
--- /dev/null
+++ b/docs/developer_portal/testing/testing-guidelines.md
@@ -0,0 +1,129 @@
+---
+title: Testing Guidelines and Best Practices
+sidebar_position: 1
+---
+
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+# Testing Guidelines and Best Practices
+
+We feel that tests are an important part of a feature and not an additional or optional effort. That's why we colocate test files with functionality and sometimes write tests upfront to help validate requirements and shape the API of our components. Every new component or file added should have an associated test file with the `.test` extension.
+
+We use Jest, React Testing Library (RTL), and Cypress to write our unit, integration, and end-to-end tests. For each type, we have a set of best practices/tips described below:
+
+## Jest
+
+### Write simple, standalone tests
+
+The importance of simplicity is often overlooked in test cases. Clear, dumb code should always be preferred over complex ones. The test cases should be pretty much standalone and should not involve any external logic if not absolutely necessary. That's because you want the corpus of the tests to be easy to read and understandable at first sight.
+
+### Avoid nesting when you're testing
+
+Avoid the use of `describe` blocks in favor of inlined tests. If your tests start to grow and you feel the need to group tests, prefer to break them into multiple test files. Check this awesome [article](https://kentcdodds.com/blog/avoid-nesting-when-youre-testing) written by [Kent C. Dodds](https://kentcdodds.com/) about this topic.
+
+### No warnings!
+
+Your tests shouldn't trigger warnings. This is really common when testing async functionality. It's really difficult to read test results when we have a bunch of warnings.
+
+### Example
+
+You can find an example of a test [here](https://github.com/apache/superset/blob/e6c5bf4/superset-frontend/src/common/hooks/useChangeEffect/useChangeEffect.test.ts).
+
+## React Testing Library (RTL)
+
+### Keep accessibility in mind when writing your tests
+
+One of the most important points of RTL is accessibility and this is also a very important point for us. We should try our best to follow the RTL [Priority](https://testing-library.com/docs/queries/about/#priority) when querying for elements in our tests. `getByTestId` is not viewable by the user and should only be used when the element isn't accessible in any other way.
+
+### Don't use `act` unnecessarily
+
+`render` and `fireEvent` are already wrapped in `act`, so wrapping them in `act` again is a common mistake. Some solutions to the warnings related to `act` might be found [here](https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning).
+
+### Be specific when using *ByRole
+
+By using the `name` option we can point to the items by their accessible name. For example:
+
+```jsx
+screen.getByRole('button', { name: /hello world/i })
+```
+
+Using the `name` property also avoids breaking the tests in the future if other components with the same role are added.
+
+### userEvent vs fireEvent
+
+Prefer the [user-event](https://github.com/testing-library/user-event) library, which provides a more advanced simulation of browser interactions than the built-in [fireEvent](https://testing-library.com/docs/dom-testing-library/api-events/#fireevent) method.
+
+### Usage of waitFor
+
+- [Prefer to use `find`](https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#using-waitfor-to-wait-for-elements-that-can-be-queried-with-find) over `waitFor` when you're querying for elements. Even though both achieve the same objective, the `find` version is simpler and you'll get better error messages.
+- Prefer to use only [one assertion at a time](https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#having-multiple-assertions-in-a-single-waitfor-callback). If you put multiple assertions inside a `waitFor` we could end up waiting for the whole block timeout before seeing a test failure even if the failure occurred at the first assertion. By putting a single assertion in there, we can improve on test execution time.
+- Do not perform [side-effects](https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#performing-side-effects-in-waitfor) inside `waitFor`. The callback can be called a non-deterministic number of times and frequency (it's called both on an interval as well as when there are DOM mutations). So this means that your side-effect could run multiple times.
+
+### Example
+
+You can find an example of a test [here](https://github.com/apache/superset/blob/master/superset-frontend/src/dashboard/components/PublishedStatus/PublishedStatus.test.tsx).
+
+## Cypress
+
+### Prefer to use Cypress for e2e testing and RTL for integration testing
+
+Here it's important to make the distinction between e2e and integration testing. This [article](https://www.onpathtesting.com/blog/end-to-end-vs-integration-testing) gives an excellent definition:
+
+> End-to-end testing verifies that your software works correctly from the beginning to the end of a particular user flow. It replicates expected user behavior and various usage scenarios to ensure that your software works as whole. End-to-end testing uses a production equivalent environment and data to simulate real-world situations and may also involve the integrations your software has with external applications.
+
+> A typical software project consists of multiple software units, usually coded by different developers. Integration testing combines those software units logically and tests them as a group.
+> Essentially, integration testing verifies whether or not the individual modules or services that make up your application work well together. The purpose of this level of testing is to expose defects in the interaction between these software modules when they are integrated.
+
+Do not use Cypress when RTL can do it better and faster. Many of the Cypress tests that we have right now, fall into the integration testing category and can be ported to RTL which is much faster and gives more immediate feedback. Cypress should be used mainly for end-to-end testing, replicating the user experience, with positive and negative flows.
+
+### Isolated and standalone tests
+
+Tests should never rely on other tests to pass. This might be hard when a single user is used for testing as data will be stored in the database. At every new test, we should reset the database.
+
+### Cleaning state
+
+Cleaning the state of the application, such as resetting the DB, or in general, any state that might affect consequent tests should always be done in the `beforeEach` hook and never in the `afterEach` one as the `beforeEach` is guaranteed to run, while the test might never reach the point to run the `afterEach` hook. One example would be if you refresh Cypress in the middle of the test. At this point, you will have built up a partial state in the database, and your clean-up function will [...]
+
+### Unnecessary use of `cy.wait`
+
+- Unnecessary when using `cy.request()` as it will resolve when a response is received from the server
+- Unnecessary when using `cy.visit()` as it resolves only when the page fires the load event
+- Unnecessary when using `cy.get()`. When the selector should wait for a request to happen, aliases would come in handy:
+
+```js
+cy.intercept('GET', '/users', [{ name: 'Maggy' }, { name: 'Joan' }]).as('getUsers')
+cy.get('#fetch').click()
+cy.wait('@getUsers') // <--- wait explicitly for this route to finish
+cy.get('table tr').should('have.length', 2)
+```
+
+### Accessibility and Resilience
+
+The same accessibility principles in the RTL section apply here. Use accessible selectors when querying for elements. Those principles also help to isolate selectors from eventual CSS and JS changes and improve the resilience of your tests.
+
+## References
+
+- [Avoid Nesting when you're Testing](https://kentcdodds.com/blog/avoid-nesting-when-youre-testing)
+- [RTL - Queries](https://testing-library.com/docs/queries/about/#priority)
+- [Fix the "not wrapped in act(...)" warning](https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning)
+- [Common mistakes with React Testing Library](https://kentcdodds.com/blog/common-mistakes-with-react-testing-library)
+- [ARIA Roles](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles)
+- [Cypress - Best Practices](https://docs.cypress.io/guides/references/best-practices)
+- [End-to-end vs integration tests: what's the difference?](https://www.onpathtesting.com/blog/end-to-end-vs-integration-testing)
diff --git a/docs/sidebarTutorials.js b/docs/sidebarTutorials.js
index 4a59018adb..97ef830c87 100644
--- a/docs/sidebarTutorials.js
+++ b/docs/sidebarTutorials.js
@@ -42,32 +42,24 @@ const sidebars = {
'contributing/howtos',
'contributing/release-process',
'contributing/resources',
+ 'guidelines/design-guidelines',
{
type: 'category',
- label: 'Contribution Guidelines',
+ label: 'Frontend Style Guidelines',
collapsed: true,
items: [
- 'guidelines/design-guidelines',
- {
- type: 'category',
- label: 'Frontend Style Guidelines',
- collapsed: true,
- items: [
- 'guidelines/frontend-style-guidelines',
- 'guidelines/frontend/component-style-guidelines',
- 'guidelines/frontend/emotion-styling-guidelines',
- 'guidelines/frontend/testing-guidelines',
- ],
- },
- {
- type: 'category',
- label: 'Backend Style Guidelines',
- collapsed: true,
- items: [
- 'guidelines/backend-style-guidelines',
- 'guidelines/backend/dao-style-guidelines',
- ],
- },
+ 'guidelines/frontend-style-guidelines',
+ 'guidelines/frontend/component-style-guidelines',
+ 'guidelines/frontend/emotion-styling-guidelines',
+ ],
+ },
+ {
+ type: 'category',
+ label: 'Backend Style Guidelines',
+ collapsed: true,
+ items: [
+ 'guidelines/backend-style-guidelines',
+ 'guidelines/backend/dao-style-guidelines',
],
},
],
@@ -102,6 +94,7 @@ const sidebars = {
collapsed: true,
items: [
'testing/overview',
+ 'testing/testing-guidelines',
'testing/frontend-testing',
'testing/backend-testing',
'testing/e2e-testing',