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: reorder a rotation in which a user participates more than once #1479

Merged
merged 8 commits into from
May 11, 2021

Conversation

dctalbot
Copy link
Contributor

  • Identified the issue which this PR solves.
  • Read the CONTRIBUTING document.
  • Code builds clean without any errors or warnings.
  • Added appropriate tests for any new functionality.
  • All new and existing tests passed.
  • Added comments in the code, where necessary.
  • Ran make check to catch common errors. Fixed any that came up.

Description:
Fixes #1478

@mastercactapus
Copy link
Member

Using an index causes subtle issues, especially w.r.t. keyboard use and focus:
https://github.com/atlassian/react-beautiful-dnd/blob/master/docs/guides/identifiers.md#avoid-reusing-ids

@dctalbot
Copy link
Contributor Author

dctalbot commented May 3, 2021

@mastercactapus good catch. I'm a bit stumped on what to do here. We could potentially expose rotation participant ID's from graphql. Any ideas?

@mastercactapus
Copy link
Member

Yeah, I spent an hour or two last week trying to come up with something. Participant IDs wouldn't work either because we update those entries, rather than reorder them, in the backend. We'll need to persist reorder history or something and match it up to get stable IDs of some sort.

Maybe this would work:

  • create a list of "ids" based on index
  • hold the most recent reorder in state
  • apply the recent reorder to the list of "ids"

I think it might work if we just handle the most recent action. That way we don't lose focus on the current element from it being replaced due to a key change.

@dctalbot dctalbot marked this pull request as draft May 4, 2021 15:12
@dctalbot dctalbot marked this pull request as ready for review May 5, 2021 19:26
@dctalbot dctalbot requested a review from mastercactapus May 5, 2021 19:31
@Forfold
Copy link
Contributor

Forfold commented May 7, 2021

Works great! Just had the one thing about maintenance then it looks good to go. Crazy that this has been present for so long 🤯

@mastercactapus mastercactapus merged commit d3669e7 into master May 11, 2021
@mastercactapus mastercactapus deleted the fix-rotation-user-list branch May 11, 2021 18:19
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.

ui/rotations: ability to reorder a rotation in which a user participates more than once
3 participants