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

Do not sort list items alphabetically #1047

Merged
merged 11 commits into from
May 28, 2024

Conversation

AlexanderSkrock
Copy link
Contributor

@AlexanderSkrock AlexanderSkrock commented Apr 26, 2024

This pull request aims to fix #311 in properties-panel together #353 in properties-panel. Currently, it is being developed.

  • Remove "shouldSort" property where ever the flag was set to false explicitly
  • Implement sorting where necessary
    => Potentially every component which did not explicitly set shouldSort to false, but we should use this opportunity to evaluate where it actually should be sorted.
  • Write a note in CHANGELOG.md

Regarding the shouldSort flag, I saw the following throughout this project.

Camunda 7 - Camunda Platform
Usages without the shouldSort flags, thus using the previous default of sorting:

  • FieldInjectionGroup
  • InMappingGroup
  • OutMappingGroup
  • ExecutionListenerGroup
  • TaskListenerGroup
  • ConnectorInputGroup
  • ConnectorOutputGroup
  • ExtensionPropertiesGroup

Usages with the shouldSort flag set to false, thus already disabling sorting:

  • ErrorsGroup
  • FormDataGroup
  • InputGroup
  • OutputGroup
  • ProcessVariablesGroup
  • ExtensionPropertiesGroup

Camunda 8 - Zeebee
Usages with the shouldSort flag set to false, thus already disabling sorting:

  • ExtensionPropertiesGroup
  • HeaderGroup
  • InputGroup
  • OutputGroup

Related to bpmn-io/properties-panel#311

@AlexanderSkrock
Copy link
Contributor Author

Hi @nikku ,
it would be great if you could have a look as well regarding the question which components should keep alphabetical sorting. Thank you in advance.

@nikku
Copy link
Member

nikku commented Apr 29, 2024

@AlexanderSkrock I can confirm that non of the fields you mentioned should be sorting.

We can argue about "pure visual only", such as C7 process variables; but then again we can sort on the business logic level.

@AlexanderSkrock AlexanderSkrock marked this pull request as ready for review April 29, 2024 07:44
@nikku nikku self-requested a review May 2, 2024 12:32
@AlexanderSkrock
Copy link
Contributor Author

Seems like my changes from properties-panel were not used. I did one more little fix there for the change dot appearing too late, but I can not confirm the remaining failures locally.

nikku
nikku previously requested changes May 6, 2024
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Auto-focus seems to need adjustment in different places (I sketched the change on one example on top of this PR); List#compareFn could be dropped (I pushed that change on top of this PR).

Cf. bpmn-io/properties-panel#353 (comment) for details.

@nikku nikku mentioned this pull request May 7, 2024
@AlexanderSkrock
Copy link
Contributor Author

AlexanderSkrock commented May 7, 2024

Thank you for having a look! I tried to follow up on your changes regarding auto-focus, but for the ListProps it seems non-trivial as there is no data-entry-id yet, I could hook into.

I wanted to add autoFocusEntry with [data-entry-id="${id}-listItem-${items.length - 1}"] input because that is how its done for all other components, but then I'd need to set the data-entry-id on a very high level, e. g. here.

Otherwise if a data-entry-id with a value-suffix would be okay such as "${id}-listItem-${items.length - 1}-value", then we could incorporate this in the SimpleEntry component in properties-panel core project, e. g. here. Btw, I wondered why we add a prefix to the id there, which we don't for all other entries.

In addition to both of these approaches with the current SimpleEntry component, I could imagine to simply switch to TextField for entries of ListProps, which should work out of the box in combination with autoFocusEntry property set to [data-entry-id="${id}-listItem-${items.length - 1}"] input. (I did not try this one out, yet.)

Would be great, if you could have a look here again, @nikku , as I am not sure which is the best approach. While the last one mentioned seems best to me at first glance, it might have non-obvious changes I am not aware of.

@nikku
Copy link
Member

nikku commented May 8, 2024

@AlexanderSkrock I'll only be able to follow-up on your comments next week.

@nikku nikku added the needs review Review pending label May 22, 2024 — with bpmn-io-tasks
@nikku
Copy link
Member

nikku commented May 22, 2024

@AlexanderSkrock I was able to review your changes, great work! The only thing missing is the auto focus which is now broken in some cases; I fixed it with some follow-up commits.

Was not fully able to grasp some of your thoughts in #1047 (comment). I'd propose to make auto-focus work in the most simple way 🙃. This implies:

  • Use the data-entry-id hack for nested lists that return a specific component (a3e3bf4)
  • Use simple entry addressing id + '-sub-entry-key' for flat lists (db441b8, dc09c9e)

Of course we could refactor further; but let's take things one step at a time.

@nikku
Copy link
Member

nikku commented May 22, 2024

Two things missing from my end to consider to get this merged:

  • Ensure that properties edited are always in view (cf. screen capture below)
  • Rebase on current main branch to resolve conflicts

capture FDpjH5_optimized

@nikku
Copy link
Member

nikku commented May 22, 2024

To be clear: I regard data-entry-** auto-focussing as a hack, in a perfect world we'd refactor the list entry to allow passing a dedicated sub-entry property (just a suffix), or provide other resolutions to the common "simply define what to auto-focus" problem:

  • We could define an implicit auto-focus, without any explicit configuration - entries can be tagged, and upon creation / "focus" the first entry that is tagged as "could have auto-focus" receives it.
  • We could rework the simple and list entries to be able to pass focus in another way.

But then again, looking at the small amount of changes needed here I'm fine to accept the contribution as is.

@nikku nikku self-requested a review May 23, 2024 07:32
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 23, 2024
@nikku nikku added the needs review Review pending label May 24, 2024 — with bpmn-io-tasks
@nikku
Copy link
Member

nikku commented May 28, 2024

I'll look into rebasing this. Thanks for your work on this issue 👏

@nikku
Copy link
Member

nikku commented May 28, 2024

Updated to build on top of @bpmn-io/properties-panel@3.19.0.

@nikku
Copy link
Member

nikku commented May 28, 2024

I can reproduce the test single failure locally and am investigating.

@nikku
Copy link
Member

nikku commented May 28, 2024

Addressed everything mentioned in #1047 (comment). Waiting for CI to pass.

CHANGELOG.md Outdated Show resolved Hide resolved
@fake-join fake-join bot merged commit b6c48fe into bpmn-io:main May 28, 2024
4 checks passed
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 28, 2024
@nikku
Copy link
Member

nikku commented May 28, 2024

Thanks for your contribution @AlexanderSkrock. We really apprechiate it 🏅 👏

@nikku nikku changed the title Remove unsupported flag "shouldSort" Do not sort list items alphabetically May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Do not sort entries alphabetically
2 participants