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

Keep row prefix constant when reordering #3176

Merged
merged 9 commits into from
Oct 17, 2023

Conversation

fabian-bizfactory
Copy link
Collaborator

No description provided.

@fabian-bizfactory fabian-bizfactory self-assigned this Sep 19, 2023
@fabian-bizfactory fabian-bizfactory marked this pull request as ready for review September 25, 2023 07:48
Copy link
Collaborator

@Kadrian Kadrian left a comment

Choose a reason for hiding this comment

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

Hmm, what you'd be doing here is "hard-coding" the name row_prefix. So rowPrefixField is an optional select field with a dynamic name, and in this case it might be row_prefix, but in other fields in might be another name, so we'll have to rely on field.rowPrefixField.name for the prefix field name.

Guess what I'd try instead would be to

  • pass down a rowPrefixFieldname and maybe the row itself from the FormConceptGroup to the FormConceptNode
  • add both to the dragContext
  • get them both from the dragContext in dropBetween
  • if defined, get the prefix field value from the row + rowPrefixFieldname and pass a modified newValue to insertValue()

Does that make sense?

@fabian-bizfactory
Copy link
Collaborator Author

Hmm, what you'd be doing here is "hard-coding" the name row_prefix. So rowPrefixField is an optional select field with a dynamic name, and in this case it might be row_prefix, but in other fields in might be another name, so we'll have to rely on field.rowPrefixField.name for the prefix field name.

Guess what I'd try instead would be to

* pass down a `rowPrefixFieldname` and maybe the `row` itself from the FormConceptGroup to the FormConceptNode

* add both to the dragContext

* get them both from the dragContext in dropBetween

* if defined, get the prefix field value from the row + rowPrefixFieldname and pass a modified `newValue` to `insertValue()`

Does that make sense?

I just implemented some changes to this PR. rowPrefixFieldname is only passed. I don't think the row needs to be passed props.value[movedFromAndIdx] returns the row as well.

Please let me know what you think

@Kadrian
Copy link
Collaborator

Kadrian commented Sep 26, 2023

Would that work with concepts dragged in from other fields as well?

@fabian-bizfactory
Copy link
Collaborator Author

No, but that wasn't possible in the previous version either. If we would allow that we would need to take in the possible values for the prefix into account, wouldn't we? To catch the case of the prefixes having different possible values.

const modifiedValue = newValue as unknown as {
[index: string]: string;
};
if (
Copy link
Collaborator

Choose a reason for hiding this comment

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

puuh, das sieht gut so aus, aber kP ob das bei dir Nebeneffekte hat, die nicht offensichtlich sind.

zB darf newValue mutable behandelt werden?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Danke, da hast du Recht, dass prop sollte so eigentlich nicht geändert werden. Ich habe gerade eine Version gepusht die ein deep copy umsetzt (mit json)

Copy link
Collaborator

@Kadrian Kadrian left a comment

Choose a reason for hiding this comment

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

@fabian-bizfactory I've cleaned up some of the logic, check my last commit. Was a bit easier than trying to explain. Looks good otherwise!

@fabian-bizfactory fabian-bizfactory merged commit e6f3253 into develop Oct 17, 2023
5 checks passed
@delete-merged-branch delete-merged-branch bot deleted the fix/row-prefix-gets-discarded-on-reorder branch October 17, 2023 06:28
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.

None yet

3 participants