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

feat(ListGroup): improve configuration options #106

Merged
merged 2 commits into from
Oct 8, 2021

Conversation

MaxTru
Copy link
Contributor

@MaxTru MaxTru commented Oct 6, 2021

  • Place items on bottom if sorting is disabled
  • Make it possible to disable autoOpening via new parameter autoOpen

related to #96

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Oct 6, 2021
@MaxTru MaxTru marked this pull request as ready for review October 6, 2021 18:56
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Oct 6, 2021
src/components/ListGroup.js Outdated Show resolved Hide resolved
@barmac
Copy link
Member

barmac commented Oct 7, 2021

It does not only change the configuration, but also the behaviour when you add new items. Note that without the proposed changes, the newly added items are stacked one on another. This means that if I add two items, the first one of these takes position no. 2. Check out this footage:

old-listener.mp4

With this PR, only the newly added item (singular) is placed on top while the item added previously is placed as in the "sorted" order. So if add two items, the former one can be placed anywhere within the "old" items. Check out the footage:

listener.mp4

So if we merge this, it will behave differently than the nested list.

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

Check out the comment above.

@MaxTru
Copy link
Contributor Author

MaxTru commented Oct 8, 2021

It does not only change the configuration, but also the behaviour when you add new items. Note that without the proposed changes, the newly added items are stacked one on another. This means that if I add two items, the first one of these takes position no. 2. Check out this footage:

old-listener.mp4
With this PR, only the newly added item (singular) is placed on top while the item added previously is placed as in the "sorted" order. So if add two items, the former one can be placed anywhere within the "old" items. Check out the footage:

listener.mp4
So if we merge this, it will behave differently than the nested list.

So you mean it should only sort in case we close / open again?

@barmac
Copy link
Member

barmac commented Oct 8, 2021

At least that's how it is implemented in nested list and the current version of list group.

@andreasgeier
Copy link

So you mean it should only sort in case we close / open again?

Yes. Sort only kicks in on close/open or when switching element selection. The requirement behind this is that list items must not jump when adding or editing items.

* Place items on bottom if sorting is disabled
* Make it possible to disable autoOpening via new parameter `autoOpen`

related to #96
@MaxTru
Copy link
Contributor Author

MaxTru commented Oct 8, 2021

Thanks @andreasgeier and @barmac for the hint and explanation. With the support of @pinussilvestrus this was fixed:
SortingOnAdd

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

It works as expected. I created a follow-up issue for an a11y issue I noticed: #107. It is not a regression.

Copy link

@andreasgeier andreasgeier left a comment

Choose a reason for hiding this comment

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

✅   LGTM

@MaxTru MaxTru merged commit a6b57f1 into main Oct 8, 2021
@MaxTru MaxTru deleted the 96-split-up-list-behaviors branch October 8, 2021 12:34
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Oct 8, 2021
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.

None yet

4 participants