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

fix <TextEdit /> bugs #1380

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

fix <TextEdit /> bugs #1380

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5736f41
revert: reset buttons
AshotN Jan 14, 2023
877d6b5
Merge branch 'SoftwareBrothers:master' into an/betterNull
AshotN Jan 16, 2023
bc38364
refactor: fix <TextEdit /> bugs
AshotN Jan 16, 2023
fc5f606
chore: remove currency-input-wrapper.tsx
AshotN Jan 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/frontend/components/property-type/array/edit.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,12 @@ type ItemRendererProps = {
}

const ItemRenderer: React.FC<EditProps & ItemRendererProps> = (props) => {
const { ItemComponent, property, onDelete, index, record, isDraggable } = props
const uniqueDraggableId = window.btoa(unescape(encodeURIComponent(`${JSON.stringify(flat.get(record.params, property.path))}-${property.path}`)))
const { ItemComponent, property, onDelete, index, isDraggable } = props

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 •
edited
Loading

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.

index={index}
key={uniqueDraggableId}
isDragDisabled={!isDraggable}
>
{(provided): JSX.Element => (
Expand Down
28 changes: 1 addition & 27 deletions src/frontend/components/property-type/base-property-component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Box, Icon, Tooltip } from '@adminjs/design-system'
import { Box } from '@adminjs/design-system'
import { ReactComponentLike } from 'prop-types'
import React, { useMemo, useState } from 'react'

Expand Down Expand Up @@ -143,32 +143,6 @@ const BasePropertyComponent: React.FC<BasePropertyComponentProps> = (props) => {
return (
<ErrorBoundary>
<Box data-css={contentTag} data-testid={testId}>
{where === 'edit' && (
<Box mt="sm" display="inline-flex">
<Tooltip direction="right" title="Reset back to original value" size="lg">
<Box>
<Icon
icon="Reset"
color={propValue === originalValue ? 'grey40' : 'grey100'}
onClick={() => {
onChange!(property.path, originalValue)
}}
/>
</Box>
</Tooltip>
<Tooltip direction="right" title="Set value to null" size="lg">
<Box>
<Icon
icon="SubtractAlt"
color={propValue === null ? 'grey40' : 'grey100'}
onClick={() => {
onChange!(property.path, null)
}}
/>
</Box>
</Tooltip>
</Box>
)}
<Component
property={property}
resource={resource}
Expand Down