Skip to content

Conversation

@frankieyan
Copy link
Member

@frankieyan frankieyan commented Nov 2, 2025

Short description

This is a second attempt at updating @doist/eslint-config to v12.0, which contains eslint-plugin-react-hooks v7.0, replacing the first attempt at #979.

Reference: https://github.com/Doist/eslint-config/pull/131

The difference is that we're now disabling no-restricted-imports and keeping react/react-in-jsx-scope after having issues bringing in React's JSX runtime. Since the exports appear to be different between React 17 and 18+, it may be easier to attempt this again after dropping React 17 support here.

Note

It should be easier to review commit-by-commit and omit certain ones, such as when prettier or import sorting are applied.

Please pay extra attention to the last few commits where eslint errors are fixed, e.g. setState and refs access during render.

Test plan

  • In the TextField component's story, change the maxLength prop and verify that it is respected, and an indicator is rendered
  • In the Modal component's story, verify that the modal's content receives focus on first render, rather than the close button. Then, verify that you can traverse to the close button via the keyboard
  • In the KeyCapturer component's story, verify that the actions tab show the appropriate key events
    • Switch to either a Japanese, Korean, or Chinese input method, enter composition mode (e.g. cmd + space then start typing) and press enter. The enter key's event should not be logged until composition mode ends

@frankieyan frankieyan requested a review from nats12 November 2, 2025 02:51
@frankieyan frankieyan added the 🙋 Ask PR Used for PRs that need a review before merging. label Nov 2, 2025
@frankieyan frankieyan requested a review from Copilot November 2, 2025 03:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the project's linting and code formatting tooling by upgrading ESLint, TypeScript ESLint, Prettier, and related configurations to their latest versions. It also applies automatic formatting fixes across the entire codebase to comply with the new rules.

Key changes:

  • Upgrades ESLint, TypeScript ESLint, and Prettier to latest major versions
  • Reorganizes TypeScript configurations (moves stories config, excludes stories from main config)
  • Applies code formatting fixes: import sorting, blank line standardization, trailing comma additions, indentation fixes

Reviewed Changes

Copilot reviewed 132 out of 139 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
package.json Updates linting tool versions to latest
.eslintrc Updates ESLint config to use new recommended ruleset names
tsconfig.stories.json Deleted - moved to stories/tsconfig.json
stories/tsconfig.json New TypeScript config for stories directory
tsconfig.json Excludes stories directory from compilation
src/**/*.tsx Formatting fixes: import sorting, blank lines, trailing commas
stories/**/*.tsx Formatting fixes: import sorting, blank lines
README.md, CHANGELOG.md, etc. Markdown list formatting standardization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@nats12
Copy link
Contributor

nats12 commented Nov 3, 2025

In the KeyCapturer component's story, verify that the actions tab show the appropriate key events

@frankieyan KeyCapturer doesn't have an actions tab 🤔

Screenshot 2025-11-03 at 10 38 32

Did you mean KeyboardShortcut? If so, the actions area is empty for me there.

Switch to either a Japanese, Korean, or Chinese input method, enter composition mode (e.g. cmd + space then start typing) and press enter. The enter key's event should not be logged until composition mode ends

This triggers Spotlight search on Mac. Unless I'm missing something relating to the actions.

[maxLength, value],
)
if (value !== previousValue || maxLength !== previousMaxLength) {
setPreviousValue(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

State is now set in the body of the component, is this safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

const propagateEvent = props[keyPropagatePropMapping[key]] || false
const eventHandler = props[keyEventHandlerMapping[key]]

if (key === 'Enter' && eventHandler) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given we're checking again for eventHandler on line 184, we could probably invert this check and check for eventHandler first, then whether key is enter:

if(eventHandler) {

	if(key === 'Enter' & ( isComposing ||
	  // Safari fires the onCompositionEnd event before the keydown event, so we
	  // have to rely on the 229 keycode, which is Enter when fired from an IME
	  // https://www.w3.org/TR/uievents/#determine-keydown-keyup-keyCode
	  (event.keyCode || event.which) === 229)
	) {
	
		return 
	}


	eventHandler(event)
     if (!propagateEvent) {
          event.preventDefault()
          event.stopPropagation()
      }
}

@frankieyan
Copy link
Member Author

In the KeyCapturer component's story, verify that the actions tab show the appropriate key events

@frankieyan KeyCapturer doesn't have an actions tab 🤔

The bottom tabs are all buried in the "Canvas" tab as they are hidden from the docs page. Sorry, I should've mentioned this in the test plan.

Switch to either a Japanese, Korean, or Chinese input method, enter composition mode (e.g. cmd + space then start typing) and press enter. The enter key's event should not be logged until composition mode ends

This triggers Spotlight search on Mac. Unless I'm missing something relating to the actions.

You'll have to have one of those input methods installed first. Here's a recording:

Screen.Recording.2025-11-03.at.10.27.59.AM.mov

@frankieyan
Copy link
Member Author

KeyCapturer doesn't have an actions tab 🤔

Also just in case, it looks like the changes you're seeing here are old, as the title has been changed from "Component Documentation" to "KeyCapturer" in this PR:

https://github.com/Doist/reactist/blob/eb6f2e09b4d400e970189316dee9486bc637edea/stories/components/KeyCapturer.stories.mdx

image

@frankieyan frankieyan requested a review from nats12 November 3, 2025 18:33
Copy link
Contributor

@nats12 nats12 left a comment

Choose a reason for hiding this comment

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

OK after seeing the video I totally got what you meant by composition mode 👍🏼 I had a Japanese input method installed but it does seem I was also looking at an older version.

Looks good! 🚀

@frankieyan frankieyan changed the title chore: Bump eslint-related dependencies and fix errors build: Bump eslint-related dependencies and fix errors Nov 4, 2025
@frankieyan frankieyan merged commit 67d7d0d into main Nov 4, 2025
9 checks passed
@frankieyan frankieyan deleted the frankie/update-eslint-dependencies-2 branch November 4, 2025 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🙋 Ask PR Used for PRs that need a review before merging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants