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

fix <TextEdit /> bugs #1380

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

AshotN
Copy link
Contributor

@AshotN AshotN commented Jan 14, 2023

This is a re-attempt at #1356

fix: array item re-render issue
Copy link
Contributor

@dziraf dziraf left a comment

Choose a reason for hiding this comment

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

@AshotN I think this PR is missing the TextEdit changes as we reverted them? Refering to PR title.


return (
<Draggable
draggableId={uniqueDraggableId}
draggableId={property.path}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this fix the issue? property.path is still not unique as it changes when you drag an element between indices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point will have to dive into that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There doesn't seem to be a convenient value to use as a unique key.

Copy link
Contributor

@dziraf dziraf Jan 17, 2023

Choose a reason for hiding this comment

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

One solution I thought of would be to create a "virtual" property draggableId for all existing array elements when the HOC component first loads, then assign it also when you add new elements. We could use some kind of library like nanoid to create a unique identifier. There could be an issue where AdminJS also sends this property in request payload and an adapter tries to save it. AFAIK some ORMs skip attributes that are not defined in models when generating db queries but other ones could throw errors. A solution to this could be that we treat all properties prefixed with double underscore (__typename, __draggableId, etc) as virtual/readonly and remove them when form is submitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you want that solution as part of this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, yes. If you have doubts though and property.path change works exactly the same as uniqueDraggableId (as it was before) then it's fine to do it another time.

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 would prefer handling it in a separate PR, the current implementation has some quirks like focus going to the wrong field when dragging elements. But that is an existing issue.

@AshotN
Copy link
Contributor Author

AshotN commented Jan 16, 2023

You're right will add that back into the PR

@AshotN
Copy link
Contributor Author

AshotN commented Jan 19, 2023

Currently AdminJS has a lot of different components for different types of data, they have repeating functionality. This PR doesn't address all of them. If the changes so far seem sound, I can continue to try to implement the same for the rest of the property-type components.

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.

2 participants