You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/09/01 18:51:01 UTC
[GitHub] [superset] etr2460 opened a new issue #16553: [Proposal] Superset Frontend Style Guidelines
etr2460 opened a new issue #16553:
URL: https://github.com/apache/superset/issues/16553
The following is a proposal for high level frontend style guidelines, distilled from the many past SiPs and proposals. These high level guidelines will be the cover page for Superset's Frontend Style Guide, and we'll propose further Dos and Don'ts that conform to these guidelines and provide more actionable details for contributors. This proposal will be posted to the dev@ email list for lazy consensus before being migrated to the Superset Wiki.
---
# Proposed Superset Frontend Style Guidelines
This is 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.
- We prefer functional components to class components, and use hooks for local component state.
- We use Ant Design components from our component library whenever possible, only building our own custom components when it's required.
- We use @emotion to provide styling for our components.
- We co-locate styling within component files.
- We use Jest for unit tests, React Testing Library for component tests, and Cypress for end to end tests.
- 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 es-lint 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!
## References
### [[SIP-36] Proposal for standardizing use of TypeScript](https://github.com/apache/superset/issues/9101)
This SIP formally adopted TypeScript as the main language for Superset frontend, started migration work for JavaScript => TypeScript, and added the requirement that all new frontend JavaScript should be written in TypeScript instead.
### [[SIP-37] Proposal to implement CSS-in-JS using Emotion](https://github.com/apache/superset/issues/9123)
This SIP formally adopted Emotion as the method for adding styling to components, describes alternatives considered, and why migration is valuable.
### [[SIP-48] Using Ant Design as our primary component library](https://github.com/apache/superset/issues/10254)
This SIP replaced Bootstrap with Ant Design as our main component library, and describes how they should interact with style overrides provided by Emotion.
### [[SIP-56] Adopt React Testing Library for testing React components](https://github.com/apache/superset/issues/11688)
This SIP replaced Enzyme with React Testing Library for testing React components, primarily because RTL is the most used testing framework and fully supports the newest React features.
### [[SIP-61] Improve the organization of front-end folders](https://github.com/apache/superset/issues/13632)
This SIP describes how we organize our frontend folders, including how we should group different types of modules and components, where tests should be located, and how files should be named.
to: @rusackas @michael-s-molina @ktmud @graceguo-supercat @eschutho @suddjian @nytai @pkdotson @betodealmeida and anyone else who works on the frontend that I may have missed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
ktmud commented on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-910615443
This is a great starting point! Do we want to have some more specific style guide, too? For example:
- React props should go with the component itself, and not in a `types.ts` file
- Don't put multiple components in a React file (should be capturable by eslint)
- Use `type ...` for Reat props and `interface ...` for all other places
- Prefer TypeScript string literals type over Enums and const variables
- How to use variables in Emotion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina edited a comment on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
michael-s-molina edited a comment on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-910626141
> Do we want to have some more specific style guide, too?
@ktmud That's the idea. In the end, what we want to achieve is a Wiki similar to this:
- Design Guidelines
- Front-end Guidelines
- Typescript
- Style/CSS/Theming
- Testing
- Linting & Syntax
- State Management
- Back-end Guidelines
Each section/page should contain specific guidelines like [this one](https://github.com/apache/superset/issues/15264) already being cooked by @rusackas that falls into the Style/CSS/Theming page.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-911552709
> where does that fall?
@eschutho @etr2460 People with experience and interest in Redux can write a proposition of dos and don'ts similar to what @rusackas did for Style/CSS [here](https://github.com/apache/superset/issues/15264). Then submit it for lazy consensus. After approval, it will become a page under Front End Guidelines. I think this will be a good process for all the topics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
eschutho commented on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-912867394
@etr2460 and @michael-s-molina thank you both for that feedback. A few of us are going to be working on cleaning up Redux state over the next few months, so we'll also start to write down some guidelines as we go along and share them out to the community to iterate on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] graceguo-supercat commented on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-910621215
thanks for bringing a Proposal. could you highlight some points that **_is_** in currently daily development, but _**preferred to be or will be**_ changed in the further by this proposal?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-910646480
>could you highlight some points that is in currently daily development, but preferred to be or will be changed in the further by this proposal?
@graceguo-supercat it's my understanding that everything in this proposal is already currently agreed to by the team and part of daily development. Let me know if that's incorrect!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud edited a comment on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-910615443
This is a great starting point! I like `If there’s not a linting rule, we don’t have a rule!`, but I think some places would still benefit from more specific rules? For example:
- React props should go with the component itself, and not in a `types.ts` file
- Don't put multiple components in a React file (should be capturable by eslint)
- Use `type ...` for React props and `interface ...` for all other places
- Prefer TypeScript string literals type over Enums and const variables
- How to use variables in Emotion
These are hard to capture in linter and formatter but could still help code maintainability, readability and performance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 closed issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
etr2460 closed issue #16553:
URL: https://github.com/apache/superset/issues/16553
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
eschutho commented on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-910882895
This is great @etr2460! I'm not sure if you want feedback on further development of some of these points, since this is high-level, but on the Redux vs local state with hooks bullet point for example, I assume we'll want to flush that out more soon. Like when is it considered "component" vs "global"? That gray area where local state is being passed around between 2-3 sub components... where does that fall? What do we do with api requests that are only used in one component... should those all be stored in redux even if only used in one component? These are some things that some of us are currently thinking about. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-910933134
Yup @eschutho , totally agree about clarifying when to use Redux vs. local state. I think we'll probably write a whole dos and don'ts for that question, although it's a bit too low level for this summary proposal.
That said, if you asked me to chime in, i'd probably say:
- state used by only one component should always be local, even if it's coming from an api (since it's probably good to have updated state in cases where a component is unrendered then rerendered)
- state used by more than one component should always be in Redux (I feel less strongly here, and could see arguments either way, but having a hard and fast rule might make our lives easier and prevent that grey area from existing)
I'm not a huge fan of Redux anyway, so i'll leave the decision making regarding state to those with more experience, more passion, and less angst 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina edited a comment on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
michael-s-molina edited a comment on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-910626141
> Do we want to have some more specific style guide, too?
@ktmud That's the idea. In the end, what we want to achieve is a Wiki similar to this:
- Design Guidelines
- Front End Guidelines
- Typescript
- Style/CSS/Theming
- Testing
- Linting & Syntax
- State Management
- Back End Guidelines
Each section/page should contain specific guidelines like [this one](https://github.com/apache/superset/issues/15264) already being cooked by @rusackas that falls into the Style/CSS/Theming page.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-910626141
> Do we want to have some more specific style guide, too?
@ktmud That's the idea. In the end, what we want to achieve is a Wiki page similar to this:
- Design Guidelines
- Front-end Guidelines
- Typescript
- Style/CSS/Theming
- Testing
- Linting & Syntax
- State Management
- Back-end Guidelines
Each section should contain specific guides like [this one](https://github.com/apache/superset/issues/15264) already being cooked by @rusackas that falls into the Style/CSS/Theming page.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina edited a comment on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
michael-s-molina edited a comment on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-910626141
> Do we want to have some more specific style guide, too?
@ktmud That's the idea. In the end, what we want to achieve is a Wiki similar to this:
- Design Guidelines
- Front-end Guidelines
- Typescript
- Style/CSS/Theming
- Testing
- Linting & Syntax
- State Management
- Back-end Guidelines
Each section should contain specific guides like [this one](https://github.com/apache/superset/issues/15264) already being cooked by @rusackas that falls into the Style/CSS/Theming page.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina edited a comment on issue #16553: [Proposal] Superset Frontend Style Guidelines
Posted by GitBox <gi...@apache.org>.
michael-s-molina edited a comment on issue #16553:
URL: https://github.com/apache/superset/issues/16553#issuecomment-910626141
> Do we want to have some more specific style guide, too?
@ktmud That's the idea. In the end, what we want to achieve is a Wiki similar to this:
- Design Guidelines
- Front-end Guidelines
- Typescript
- Style/CSS/Theming
- Testing
- Linting & Syntax
- State Management
- Back-end Guidelines
Each section/page should contain specific guides like [this one](https://github.com/apache/superset/issues/15264) already being cooked by @rusackas that falls into the Style/CSS/Theming page.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org