Skip to content

Conversation

@Muta-Jonathan
Copy link
Member

@Muta-Jonathan Muta-Jonathan commented Feb 4, 2025

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

  1. Due to this feature in the new carbon versions ... I had to upgrade our carbon version to "@carbon/react": "^1.75.0", which is the latest as per this PR
  2. Our old implementation was using the current state of formField.questionOptions?.answers which wen updated from the
setFormField((prevField) => {
        const currentAnswers = prevField.questionOptions?.answers || [];
        if (JSON.stringify(currentAnswers) === JSON.stringify(mappedAnswers)) {
          return prevField;
        }
        return {
          ...prevField,
          questionOptions: {
            ...prevField.questionOptions,
            answers: mappedAnswers,
          },
        };
      });

would trigger changes to the formfields hence making

<MultiSelect
          className={styles.multiSelect}
          direction="top"
          id="selectAnswers"
          items={answerItems}
          itemToString={convertAnswerItemsToString}
          onChange={handleSelectAnswers}
          size="md"
          selectedItems={selectedAnswers}
          titleText={t('selectAnswersToDisplay', 'Select answers to display')}
        /> 

to show only selected items as options
Now with this new implementation have introduced the useRef to store the initial state of
const initialAnswers = useRef(formField.questionOptions?.answers ?? []); so as to show all selected and unselected question answers

Screen recording

Screen.Recording.2025-02-05.at.2.02.21.AM.mov

Related Issue

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

@Muta-Jonathan Muta-Jonathan requested review from NethmiRodrigo, ibacher and pirupius and removed request for NethmiRodrigo and pirupius February 4, 2025 23:17
@NethmiRodrigo NethmiRodrigo changed the title (fix) O3-3958: Select all concept answers in Edit question Modal via the Interactive schema builder (fix) O3-3958: Allow user to select all answer options Feb 5, 2025
Copy link
Collaborator

@NethmiRodrigo NethmiRodrigo left a comment

Choose a reason for hiding this comment

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

Thanks @Muta-Jonathan! A couple of things, the select all option isn't showing up when I run this locally, so please double check this fix and include a screenshot of how it works. Additionally, you'd have to override the import map and test within the app shell and try locally linking the form engine and running because the carbon update is still pending in the form engine and might cause issues.

Copy link
Collaborator

@NethmiRodrigo NethmiRodrigo left a comment

Choose a reason for hiding this comment

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

@Muta-Jonathan the screen recording you have provided doesn't load, so please update the PR with one that demonstrates your implementation

Comment on lines +25 to +27
// Storing the initial questionOptions answers
const initialAnswers = useRef(formField.questionOptions?.answers ?? []);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Were these changes from a bad rebase?

@jayg2002
Copy link
Contributor

jayg2002 commented Mar 1, 2025

Hi @Muta-Jonathan just putting it out, in the video you have put as the implementation I see that when select all is selected and you de-select some option the button should be toggle back right? cause I see that it's not toggling back

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion would be :

  1. Add a state variable isSelectAllDisabled or something on that line
  2. Update handleSelect to accommodate the changes

(content, element) =>
content.includes('Total items selected') && content.includes('1') && content.includes('To clear selection'),
),
).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

For improved readability and maintainability, consider using an array with .every() for validating all expected texts. This way, it's easier to scale if more substrings are added in the future, just like below :

const expectedTexts = ['Total items selected', '1', 'To clear selection'];
screen.getByText((content) => expectedTexts.every((text) => content.includes(text)));

@NethmiRodrigo
Copy link
Collaborator

Closing this PR since it has been inactive and I resumed the work on #519

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.

4 participants