Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial React guidelines. #2

Merged
merged 2 commits into from
Mar 19, 2019
Merged

Conversation

bryanrsmith
Copy link
Contributor

No description provided.

## Write Focused Components
Like functions, well-designed React components should focus on a specific piece of functionality. Prefer factoring code into many small, focused components over monolithic components that cover major feature areas. Most apps tend to have a few large components that pull together several unrelated features. These should be rare, and in cases where they're needed prefer composing many smaller components rather than implementing functionality directly within the monolithic component.

Small and focused components are easier to read and understand. They're also easier to reuse if the need arrises, and they provide React with optimization opportunities because you can easily use `React.memo` to avoid unnecessary re-renders.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "arrises" -> "arises".

(I wonder how practical it would be to have a "build" for this repo that just runs a spellchecker…)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I should really use a spellchecker in markdown documents…


Many of React's exports use overly generic names, or shadow globals. e.g., `Component`, `lazy`, `MouseEvent`. Using named imports for these makes code harder to read because additional context is required to understand that these are React APIs. Hooks get an exception because they use a very specific naming convention that's unlikely to conflict, and function components are easier to read when built-in hooks don't get special treatment over custom hooks.

We write a lot of React code, and consumign the React API in a consistent way makes code easier to read, and team transitions smoother.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consumign -> consuming

import * as React from 'react';

// bad
import React, { Component, TouchEvent, useEffect } from 'react';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sufficiently easy to describe that it should be an eslint rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I think there are a few we could add rules for.

Sometimes SVGs include `id`s. If possible, ask design for an alternate implementation. If `id`s are required, use a tool or webpack plugin to ensure they are unique in the project.

## Use Focus to Dismiss Modals
Generally, the easiest way to dismiss a modal dialog or menu is with an "outer click" pattern. When the modal opens, you add a `click` event listener to body, and check if you see any clicks outside the modal, you close it. This is fundamentally broken for keyboard users, who will get stuck in dialogs they cannot dismiss. The correct way to dismiss a modal is to track focus, and close the modal when focus moves outside the modal container. It's more work, but it results in a far better user experience.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall specifics, but I'm pretty sure I've encountered instances in which this approach created some cross-browser nightmares, particularly with modals hosting fully custom UI components (maybe a calendar picker?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely harder to manage, and React doesn't help you out a lot. I hope we have better solutions to focus management in the future.

Related: reactjs/rfcs#109

@bryanrsmith bryanrsmith merged commit da4b67b into Faithlife:master Mar 19, 2019
@bryanrsmith bryanrsmith deleted the react branch March 19, 2019 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants