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) Multi-select component to initialise and display consistently in edit mode #258

Merged
merged 2 commits into from
May 7, 2024

Conversation

samuelmale
Copy link
Member

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

This aims to fix an issue with the multi-select component where it doesn't preselect previously selected values while in edit mode.

Screenshots

https://www.loom.com/share/6a7b2ac48ff7430ba25e6a75da42c1d3

Related Issue

https://openmrs.atlassian.net/browse/O3-3135

Other

@samuelmale samuelmale marked this pull request as ready for review May 7, 2024 12:38
Copy link

github-actions bot commented May 7, 2024

Size Change: -51 B (0%)

Total Size: 1.13 MB

ℹ️ View Unchanged
Filename Size Change
dist/184.js 11.2 kB 0 B
dist/225.js 2.57 kB 0 B
dist/29.js 206 kB 0 B
dist/300.js 700 B 0 B
dist/327.js 1.68 kB 0 B
dist/333.js 241 kB -25 B (0%)
dist/335.js 958 B 0 B
dist/353.js 3.02 kB 0 B
dist/41.js 3.36 kB 0 B
dist/422.js 3.03 kB 0 B
dist/505.js 6.94 kB 0 B
dist/540.js 2.63 kB 0 B
dist/546.js 4.98 kB 0 B
dist/55.js 752 B 0 B
dist/59.js 5.59 kB 0 B
dist/606.js 2.23 kB 0 B
dist/635.js 14.2 kB 0 B
dist/782.js 4.37 kB 0 B
dist/789.js 203 kB 0 B
dist/794.js 16.6 kB 0 B
dist/942.js 482 B 0 B
dist/977.js 80.8 kB 0 B
dist/99.js 692 B 0 B
dist/993.js 3.08 kB 0 B
dist/main.js 310 kB -26 B (0%)
dist/openmrs-form-engine-lib.js 3.81 kB 0 B

compressed-size-action

@gitcliff
Copy link
Contributor

gitcliff commented May 7, 2024

thanks @samuelmale LGTM

Copy link
Member

@vasharma05 vasharma05 left a comment

Choose a reason for hiding this comment

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

A few changes, else looks good to me.
Thanks!

Comment on lines +33 to +41
const selectOptions = question.questionOptions.answers
.filter((answer) => !answer.isHidden)
.map((answer, index) => ({
id: `${question.id}-${answer.concept}`,
concept: answer.concept,
label: answer.label,
key: index,
disabled: answer.disable?.isDisabled,
}));
Copy link
Member

Choose a reason for hiding this comment

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

Can we memoise this?

Copy link
Member Author

Choose a reason for hiding this comment

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

In my first approach memoised it but for some reason (will be addressed during the formik migration), some state events weren't properly being propagated. An example is the answer.disable. isDisabled. This property is computable based on skip logic, when I have it like:

const selectOptions = useMemo(
    () =>
      question.questionOptions.answers
        .filter((answer) => !answer.isHidden)
        .map((answer, index) => ({
          id: `${question.id}-${answer.concept}`,
          concept: answer.concept,
          label: answer.label,
          key: index,
          disabled: answer.disable?.isDisabled,
        })),
    [question],
  );

The new state computation of answer.disable?.isDisabled doesn't get propagated after skip logic runs. I guess it's the same reason why it wasn't memoised originally.

return selectOptions.filter((item) => field.value?.includes(item.concept));
}
return [];
}, [isFieldInitializationComplete, field.value]);
Copy link
Member

Choose a reason for hiding this comment

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

counter, setCounter and selectOptions will also be a dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well yes but we don't need to subscribe to them to evaluate the initiallySelectedQuestionItems.

  1. Subscribing to selectOptions introduces a weird bug (I had it as a dep in my initial implementation).
  2. We evaluate the state only if the counter is 0 so we don't have to subscribe to it.

@samuelmale samuelmale merged commit ae36027 into main May 7, 2024
4 checks passed
@samuelmale samuelmale deleted the fix/multi-select branch May 7, 2024 14:36
vasharma05 pushed a commit that referenced this pull request May 27, 2024
…n edit mode (#258)

* Centralise select-options state

* Use filter
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.

3 participants