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.

Sign up for GitHub

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

Jump to bottom

#8320: prevent IsolatedComponent and brick renderers from recreating components #8325

Merged
merged 5 commits into from
Apr 23, 2024
Merged

#8320: prevent IsolatedComponent and brick renderers from recreating components #8325

merged 5 commits into from
Apr 23, 2024

Conversation

grahamlangford
Copy link
Collaborator

@grahamlangford grahamlangford commented Apr 23, 2024 •
edited by twschiller
Loading

What does this PR do?

Remaining Work

Demo

Checklist

  • Add jest or playwright tests and/or storybook stories
  • Designate a primary reviewer @BLoe

@grahamlangford grahamlangford linked an issue Apr 23, 2024 that may be closed by this pull request
Search field loses focus while typing on snippet mod #8320
Closed
grahamlangford
grahamlangford commented Apr 23, 2024
View reviewed changes
src/bricks/renderers/customForm.tsx
@@ -299,24 +315,8 @@ export class CustomFormRenderer extends RendererABC {
normalizedData,
});

const CustomFormComponent: React.FunctionComponent<
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We were recreating the component on every render. I believe I've caught everywhere we were doing this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've seen this issue come up a few times in the past on my previous team as well. I wonder if there is a lint rule for this.

edit: aha, I found this:
https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unstable-nested-components.md

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, this isn't caught because this isn't within a React component... it's our own custom class definition.

@codecov Codecov
Copy link

codecov bot commented Apr 23, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 87.50000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 73.51%. Comparing base (2d180ba) to head (a7d1364).

Files Patch % Lines
src/bricks/renderers/propertyTable.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #8325   +/-   ##
=======================================
  Coverage   73.50%   73.51%           
=======================================
  Files        1330     1330           
  Lines       41089    41090    +1     
  Branches     7625     7626    +1     
=======================================
+ Hits        30202    30206    +4     
+ Misses      10887    10884    -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

grahamlangford
grahamlangford commented Apr 23, 2024
View reviewed changes
src/bricks/renderers/customForm.tsx
@@ -200,6 +200,22 @@ export const CUSTOM_FORM_SCHEMA = {
required: ["schema"],
};

const IsolatedCustomFormComponent: React.FunctionComponent<
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed each to Isolated... to make the React Dev Tools more useful.

@twschiller twschiller added this to the 1.8.13 milestone Apr 23, 2024
BLoe
BLoe approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@BLoe BLoe left a comment

Choose a reason for hiding this comment

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

Makes sense to me!

@github-actions GitHub Actions
Copy link

When the PR is merged, the first loom link found on this PR will be posted to #sprint-demo on Slack. Do not edit this comment manually.

@twschiller twschiller added bug Something isn't working regression labels Apr 23, 2024
@grahamlangford grahamlangford marked this pull request as ready for review April 23, 2024 19:16
@grahamlangford
remove page.pause
5a15aeb
@grahamlangford
Merge branch '8320-search-field-loses-focus-while-typing-on-snippet-m…
a7d1364 
…od' of github.com:pixiebrix/pixiebrix-extension into 8320-search-field-loses-focus-while-typing-on-snippet-mod
twschiller
twschiller reviewed Apr 23, 2024
View reviewed changes
end-to-end-tests/tests/regressions/formFlicker.spec.ts
const input = sideBarPage.getByPlaceholder("Search snippets");
await input.click();
// Add delay to give time for the the input to lose focus
await input.pressSequentially("abc", { delay: 100 });
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@grahamlangford grahamlangford merged commit 0907b1a into main Apr 23, 2024
25 checks passed
@grahamlangford grahamlangford deleted the 8320-search-field-loses-focus-while-typing-on-snippet-mod branch April 23, 2024 20:54
@grahamlangford grahamlangford mentioned this pull request Apr 24, 2024
Closed
@twschiller twschiller mentioned this pull request Apr 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fungairino fungairino fungairino left review comments

@twschiller twschiller twschiller left review comments

@BLoe BLoe BLoe approved these changes

Assignees

@grahamlangford grahamlangford

Labels
bug Something isn't working regression
Milestone
1.8.13
Development

Successfully merging this pull request may close these issues.

Search field loses focus while typing on snippet mod Sidebar mod panel flashing/re-render
4 participants
@grahamlangford @twschiller @BLoe @fungairino