webapp/STYLE_GUIDE.md
This is the style guide for the Mattermost Web App. It establishes strict rules and more general recommendations for how code in the app should be written. It also builds on the rules enforced by automated tooling by establishing how our code should be written in cases which either cannot be enforced by that tooling or in cases where there's room for developer discretion.
Whenever possible, we should use automated tooling (ESLint for our JavaScript/TypeScript code and Stylelint for our CSS/SCSS code) to enforce code style rules and linter checks. By making those automated, they can be enforced automatically by CI and developers' editors. That makes it easier to ensure that those checks are always applied without giving reviewers extra work, and it makes sure that they are applied evenly across the code base.
The following guidelines should be applied to both new and existing code. However, this does not mean that a developer is required to fix any surrounding code that contravenes the rules in the style guide. It's encouraged to keep fixing things as you go, but it's not compulsory to do so. Reviewers should refrain from asking for stylistic changes in surrounding code if the submitter has not included them in their pull request.
useSelector/useDispatch hooks instead of using connect for new components.useCallback should be used to reduce re-rendering. Similarly, use useMemo when constructing arrays or objects to pass into child components.React.memo to wrap components with a lot of internal logic that happens on render, but avoid it for components that are cheap to render.makeAsyncComponent wrapper to allow for code splitting to separate out heavy routes and components into separate bundles.my_component.scss should be next to my_component.tsx. Import those styles directly into the corresponding component.MyComponent).MyComponent would have the class MyComponent__title.MyComponent would be styled by putting a &.compact rule inside of .MyComponent.!important. The naming conventions above should help prevent conflicts, but if they don't, consider renaming classes or reducing the specificity of the conflicting CSS.color: var(--link-color)). Don't hard code color values in those areas. A list of all of those variables are available in channels/src/sass/base/_css_variables.scss.rgba with the versions of theme variables suffixed with -rgb (e.g. color: rgba(var(--link-color-rgb), 0.8))).GenericModal or Menu) instead of writing new components because those components should already follow standard accessibility patterns.button, input, h2, or ul instead of more general ones like div or span. Those elements are accessible by default, and more general elements need additional attributes and logic to be similarly accessible.button), a form control's corresponding label element, another element linked using the aria-labelledby attribute, or from the aria-label attribute. Prefer using methods other than aria-label when possible to ensure the accessible name matches visible labels whenever possible because some sighted users will still use accessibility tools to navigate the app.role or other aria- attributes. For example, don't include the word "button" in a button's accessible name.aria-describedby attribute or the aria-description attribute. Prefer using aria-describedby because it has wider support and because it can reuse existing help text that may already be present in the app.aria- attributes. For example, don't include the expanded/collapsed state on an element with aria-expanded on it.alt attribute or aria-label, but a user's profile picture next to their name in the UI may not.aria-label. Those are already read out by accesibility tools.role specified. They must also have any additional information defined using aria- attributes as defined in the ARIA APG. For example, a custom checkbox should have its checked state denoted using aria-checked while a menu should have its open/closed state denoted using aria-expanded.a11y--focused CSS class, but it would ideally be done using the :focus-visible pseudoclass.isKeyPressed(event, Constants.KeyCodes.KEY_NAME) instead of raw key codes to support different keyboard layouts.cmdOrCtrlPressed(event) to have shortcuts using the Ctrl key to have them use the Cmd key on MacOS.stopPropagation to handle any conflicts that do occur. If that's not possible, use event.target to ensure that the keyboard shortcut isn't firing when it shouldn't be (such as when pressing escape to close a menu also causes the parent modal to close).A11yController provides additional keyboard support for elements in the app based on their CSS class and attributes.
a11y__region class and a data-a11y-sort-order attribute to major regions of the UI to allow users to navigate between them by pressing F6 or shift+F6.a11y__section class to items in a list to make them keyboard-navigable using the arrow keys.WithTooltip, GenericModal, or a similar component, add the a11y__modal or a11y__popup class to a popup or modal to disable global navigation within it.FormattedMessage over useIntl unless you specifically need a string for a prop.MessageDescriptor objects (e.g. {id: 'test.string', defaultMessage: 'Test String}). If that's not possible, an IntlShape object may be provided.localizeMessage.utils/react_testing_utils instead of importing them directly from RTL. Use renderWithContext for any components that need access to Redux, I18n, or React Router context.userEvent.click instead of calling methods directly) and testing expected behavior by writing assertions about visible characteristics (e.g. asserting that an element is visible versus checking a component's internal state).
expect(...).toBeVisible(...) to ensure that others don't unintentionally break the component or its tests in the future.expect(...).toHaveAttribute('aria-expanded', 'true')). See the section above on accessibility for more information.getByRole > getByText/getByPlaceholderText > getByLabelText/getByAltText/getByTitle > getByTestId. Usage of getByTestId should be rare.userEvent over fireEvent for user interactions, and don't directly call methods on DOM elements to simulate events. RTL's userEvent simulates events the most realistically, and it ensures that component changes are properly wrapped in act.
userEvent as those methods are all asynchronous (e.g. await userEvent.click(...)).fireEvent only in these specific cases where userEvent cannot be used:
userEvent doesn't have direct focus/blur methods. Use fireEvent.focus() and fireEvent.blur().userEvent doesn't support scroll events. Use fireEvent.scroll().userEvent doesn't support image loading events. Use fireEvent.load() and fireEvent.error().userEvent.keyboard() requires element focus. Use fireEvent.keyDown(document, ...) for global keyboard shortcuts.userEvent doesn't work well with jest.useFakeTimers() and causes timeouts. Use fireEvent.click() when tests use fake timers.userEvent respects CSS pointer-events: none on disabled elements. Use fireEvent.click() when testing that disabled element handlers are properly guarded.userEvent.hover() only triggers mouseEnter/mouseOver, not mouseMove. Use fireEvent.mouseMove() when testing mouseMove handlers specifically.act should only be used when performing any action that causes React to update and when that action does not already go through a helper provided by RTL such as userEvent. Typically, most tests can be written without using act explicitly.GenericModal from @mattermost/components instead of React Bootstrap's Modal.WithTooltip for simple tooltips and Floating UI for more advanced usage.@mattermost/compass-icons for icons.data field or an object with an error field.Client4 should only be used directly in Redux actions. When needed, that data should be cached in Redux.Client4 should use bindClientFunc if possible for convenience and to follow standard error handling. If they don't, the Client4 call should be wrapped in a try/catch, and the catch block should contain a forceLogoutIfNecessary and dispatch a logError.DelayedDataLoader.Array.prototype.map) should be memoized by using createSelector.makeGet... factory to be memoized per-instance. This isn't necessary when a selector doesn't need to be memoized.useMemo. When a selector factory is used in a class component, the instance of the selector factory should be in a makeMapStateToProps.makeUseEntity helper to create a new hook for lazily fetching entity data from the servermattermost-redux and in state.entities, and put web app state that needs to be persisted outside of mattermost-redux and in state.views.Client4 instance, preferably in a Redux action. In cases where it needs to be bypassed, document the reason for that.Client4 class in the Client package (platform/client).These items are gaps identified in the code analysis. They are areas for active refactoring and cleanup.