Dynamically move items in QuestionnaireBuilder

[jamestouri]

Would love some UI feedback as I'm thinking of ways to move some of the actions around

Screen.Recording.2023-09-22.at.2.07.09.PM.mov

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
medplum-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 27, 2023 1:08am

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
medplum-www	⬜️ Ignored (Inspect)	Visit Preview		Sep 27, 2023 1:08am

[coveralls]

coverage: 93.973% (-0.1%) from 94.079% when pulling baaf1c3 on james-builder-move-item into dd6d9dc on main.

[codyebberson]

Thanks for taking a stab at this.

See comments about moving logic into ItemBuilder

Aesthetically, I like the icon choice. It's a little on the bold side. Maybe shrink the icons down ~25%, and make them a more subtle gray?

[codyebberson]

If we move this from QuestionnaireBuilder into ItemBuilder, then I think you should be able to simplify the reorderItems method.

Currently, reorderItems recursively searches for the item to move. If you do it in ItemBuilder, I believe you should be able to use the array directly, similar to how we do it with addItem and removeItem

[codyebberson]

I'm not sure if this is useful here, but I've found that using delta with values 1 or -1 can be more elegant than direction enums. Then it becomes less if/else, and just math.

[codyebberson]

It's not really a "toggle"... Maybe moveItem?

In this case, I think your code will end up cleaner if you accept the itemIndex rather than itemToMove. When passing in the QuestionnaireItem, you have to do an Array.find() which is O(n) lookup. You already have the item index on lines 295-296, so might as well use it directly.

[codyebberson]

We almost never return null from any of our methods, except in very rare circumstances. Usually we'd return undefined for a "nothing" result.

However, in this case, I think we should always return the QuestionnaireItem[] array. If the delta is invalid, then we can just return the original array. Fewer edges cases to worry about (you're already preventing this case by not including the buttons, and we're not checking for null return values at the call site).

As mentioned above, I think we should change itemToMove to itemIndex and pass in the numeric array index. That should avoid the findIndex call.

Then you can remove the index === -1 check.

const newIndex = itemIndex + delta;
if (newIndex < 0 || newIndex >= items.length) {
  return items;
}
...

[github-actions]

✅ prettier errors have been resolved. Thank you.

^{#9be56607bb8c3a5ad76d25be14a6dda07fd0ba7d}

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

85.4% Coverage
0.0% Duplication