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

[async-storage]: Fix inline editing cursor jump #36

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

Thomas-Gallant
Copy link
Contributor

When attempting to update a value through async storage devtools UI, the cursor is moved to the beginning of the text input.

This change disables re-rendering of the Value column cells as the user is typing. This issue no longer appears, and functionality of saving changes, adding and removing key-value pairs does not appear to be affected by this change.

Fixes #23

@lukmccall lukmccall requested a review from Kudo March 26, 2024 07:28
@Kudo
Copy link
Collaborator

Kudo commented Mar 26, 2024

@jthoward64 could you help to review the change when you get a chance?

Comment on lines 96 to 102
};
},
shouldCellUpdate(record, prevRecord) {
return record.editedValue !== prevRecord.editedValue;
shouldCellUpdate() {
return false;
},
},
{
Copy link
Contributor

@jthoward64 jthoward64 Mar 27, 2024

Choose a reason for hiding this comment

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

Suggested change
};
},
shouldCellUpdate(record, prevRecord) {
return record.editedValue !== prevRecord.editedValue;
shouldCellUpdate() {
return false;
},
},
{
onBlur() {
setEditingManually(false);
},
onFocus() {
setEditingManually(true);
},
};
},
shouldCellUpdate(record, prevRecord) {
return !editingManually && record.editedValue !== prevRecord.editedValue
},
},

I added a state to check whether the user currently has focused the cell and that does seem to fix the jump issue while still keeping the json editor working. (Come to think of it this could probably be a ref instead of state, but it probably doesn't matter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I didn't realize my change would break the json editor. I'll merge in this suggestion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the pr, like you said the json editor seems to be working now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've updated the pr, like you said the json editor seems to be working now.

In that case this looks good to me, so long as @Kudo doesn't have any issues

@jthoward64
Copy link
Contributor

If this isn't going to be merged I'm willing to pull out the async storage plugin and maintain it myself

@Kudo
Copy link
Collaborator

Kudo commented Apr 25, 2024

thanks @Thomas-Gallant and @jthoward64 for helping this change. i'm merging the pr and publish updates soon.

@Kudo Kudo merged commit d3e8fcd into expo:main Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@dev-plugin/async-storage inline editing cursor jump
3 participants