Ensure that field components do not get recreated when state changes #860
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test Results448 tests +8 444 ✔️ +8 5m 14s ⏱️ -16s Results for commit e42dc22. ± Comparison against base commit 475c42e. This pull request removes 2 and adds 10 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
Hm, looks like this breaks a couple of critical tests. I am not sure why the fields are not updating when assigning to the card instance programatically. I'm checking but have no good clue for now |
burieberry
reviewed
Nov 28, 2023
lukemelia
reviewed
Nov 28, 2023
habdelra
reviewed
Nov 30, 2023
Comment on lines
+287
to
+290
| let getComponents = () => | ||
| model.children.map((child) => | ||
| getBoxComponent(cardTypeFor(field, child), format, child, field, context), | ||
| ); // Wrap the the components in a function so that the template is reactive to changes in the model (this is essentially a helper) |
delete-merged-branch
bot
deleted the
cs-6211-containsmany-editor-field-loses-focus
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a result of pairing with @ef4 yesterday -
The issue we were solving is a bug where the "contains many" editor component will lose focus after inputting a single character, making it impossible to write a word in one go:
It turns out the whole editor component got replaced (i.e. we could see that the id in
<div id="ember102"...got replaced) in DOM after state changes. The fix is that we now cache the components by field name, using the same technique we already use ingetBoxComponent.We were thinking about the possibilities about adding a test for this, but we don't actually know enough about the source of this problem. The described bug reproduction is one way to see the bug, but we assume that this is only one case among many, and that the glimmer engine is usually smart enough to not recreate components (but not in this case)...