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) Restore ability to collapse and expand the entire form #413

Merged
merged 2 commits into from
Oct 17, 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 PR primarily restores the ability to collapse and expand all visible form pages and sections.

Out of scope:

  1. Unmounting Issue: This PR resolves an issue where toggling between expanding and collapsing a page caused its child sections to unmount from the DOM. This was addressed by removing the conditional rendering of sections within the PageRenderer.
  2. UI Enhancements: Additionally, this PR introduces UI tweaks that visually nest sections within their respective page containers.

Screenshots

Unmounting Issue:

2024-10-16 01-12-03 2024-10-16 01_13_21
(The Encounter Provider and Location fields are remounted)

Unmounting Issue Fixed:

2024-10-16 01-13-56 2024-10-16 01_15_33

Other features:

2024-10-16 01-15-45 2024-10-16 01_20_02

Related Issue

N/A

Other

Note: Carbon doesn't support nesting accordions, which is why I resorted to manipulating the visibility of the sections during page collapse; see:

Copy link

github-actions bot commented Oct 15, 2024

Size Change: -252 kB (-17.07%) 👏

Total Size: 1.22 MB

Filename Size Change
dist/751.js 0 B -252 kB (removed) 🏆
ℹ️ View Unchanged
Filename Size Change
dist/151.js 378 kB 0 B
dist/225.js 2.57 kB 0 B
dist/277.js 1.84 kB 0 B
dist/300.js 642 B 0 B
dist/335.js 968 B 0 B
dist/343.js 253 kB 0 B
dist/353.js 3.02 kB 0 B
dist/41.js 3.37 kB 0 B
dist/420.js 108 kB 0 B
dist/422.js 6.8 kB 0 B
dist/540.js 2.63 kB 0 B
dist/55.js 758 B 0 B
dist/617.js 86.9 kB 0 B
dist/635.js 14.3 kB 0 B
dist/70.js 483 B 0 B
dist/901.js 11.8 kB 0 B
dist/99.js 691 B 0 B
dist/993.js 3.09 kB 0 B
dist/main.js 342 kB +160 B (+0.05%)
dist/openmrs-esm-form-engine-lib.js 3.67 kB -1 B (-0.03%)

compressed-size-action

@samuelmale samuelmale requested a review from brandones October 16, 2024 10:04
Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

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

Great work, @samuelmale. I've left some comments.

Comment on lines 61 to 66
<CollapsibleSectionContainer
section={section}
sectionIndex={index}
visibleSections={visibleSections}
isFormExpanded={isFormExpanded}
/>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<CollapsibleSectionContainer
section={section}
sectionIndex={index}
visibleSections={visibleSections}
isFormExpanded={isFormExpanded}
/>
<CollapsibleSectionContainer
key={`section-${section.label}`}
section={section}
sectionIndex={index}
visibleSections={visibleSections}
isFormExpanded={isFormExpanded}
/>

<SectionRenderer section={section} />
</div>
</AccordionItem>
{visibleSections.map((section, index) => (
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to your changes, but maybe visibleSections could benefit from memoization:

const visibleSections = useMemo(() => {
  return page.sections.filter((section) => {
    const hasVisibleQuestions = section.questions.some((question) => !isTrue(question.isHidden));
    return !isTrue(section.isHidden) && hasVisibleQuestions;
  });
}, [page.sections]);

@@ -42,25 +55,51 @@ function PageRenderer({ page }: PageRendererProps) {
</span>
</p>
</div>
{!isCollapsed && (
<div className={isCollapsed ? styles.hiddenAccordion : styles.accordionContainer}>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<div className={isCollapsed ? styles.hiddenAccordion : styles.accordionContainer}>
<div className={classNames({
[styles.hiddenAccordion]: isCollapsed,
[styles.accordionContainer]: !isCollapsed
})}>

[styles.firstSection]: sectionIndex === 0,
[styles.lastSection]: sectionIndex === visibleSections.length - 1,
})}
key={`section-${section.label}`}>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
key={`section-${section.label}`}>
>

Copy link
Member

@pirupius pirupius left a comment

Choose a reason for hiding this comment

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

Thanks @samuelmale

@pirupius pirupius merged commit 5bd2e47 into main Oct 17, 2024
4 checks passed
@pirupius pirupius deleted the feat/restoreFormCollapseToggle branch October 17, 2024 09:50
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