(feat) Restore side navigation #417

samuelmale · 2024-10-24T12:29:51Z

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

Brings back the form's side navigation with some improvements! The nav shows up when you have multiple pages and enough screen space (extra-wide or ultra-wide workspace).

Under the Hood

Three new hooks make this work:

useFormWorkspaceSize: Figures out how much space we have to work with
usePageObserver: Keeps track of pages, errors, and what's visible
useCurrentActivePage: Handles page navigation

Known Limitations

Few things we might want to add later:

Can't jump directly to fields with errors yet
No toggle to hide/show the navigation
No "mark all as unspecified" feature

Screenshots

extra-wide workspace:

ultra-wide workspace:

wider workspace:

Related Issue

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

Other

github-actions · 2024-10-24T12:32:47Z

Size Change: -353 kB (-22%) 🎉

Total Size: 1.25 MB

Filename	Size	Change
`dist/343.js`	0 B	-253 kB (removed)	🏆
`dist/697.js`	0 B	-111 kB (removed)	🏆

ℹ️ View Unchanged

Filename	Size	Change
`dist/151.js`	379 kB	0 B
`dist/225.js`	2.57 kB	0 B
`dist/254.js`	88.7 kB	0 B
`dist/277.js`	1.85 kB	0 B
`dist/300.js`	642 B	0 B
`dist/335.js`	968 B	0 B
`dist/353.js`	3.02 kB	0 B
`dist/41.js`	3.37 kB	0 B
`dist/539.js`	262 kB	0 B
`dist/540.js`	2.63 kB	0 B
`dist/55.js`	758 B	0 B
`dist/585.js`	112 kB	0 B
`dist/635.js`	14.4 kB	0 B
`dist/690.js`	11.5 kB	0 B
`dist/70.js`	483 B	0 B
`dist/979.js`	6.87 kB	0 B
`dist/99.js`	691 B	0 B
`dist/993.js`	3.09 kB	0 B
`dist/main.js`	354 kB	+9.94 kB (+2.89%)
`dist/openmrs-esm-form-engine-lib.js`	3.8 kB	+1 B (+0.03%)

_{compressed-size-action}

denniskigen · 2024-10-24T12:59:01Z

Could we get some early design feedback on this, @samuelmale? Screenshots or videos would be ideal.

pirupius · 2024-11-01T14:53:17Z

src/components/renderer/form/form-renderer.component.tsx

+  useEffect(() => {
+    const scrollablePages = formJson.pages.filter((page) => !page.isSubform).map((page) => page);
+    pageObserver.updateScrollablePages(scrollablePages);
+  }, [formJson.pages]);


Wouldn't changing this to a useMemo be a better option for this?

I think the useEffect is ideal for this scenario.

pirupius · 2024-11-01T14:57:00Z

src/components/renderer/page/page.renderer.component.tsx

+    return () => {
+      pageObserver.removeInactivePage(page.id);
+    };


Do we still need this if it's already being handled in lines 52-53? or does this apply on initial load?

pirupius · 2024-11-01T15:03:30Z

src/components/sidebar/sidebar.component.tsx

+            </div>
+          );
+        })}
+      {sessionMode !== 'view' && <hr className={styles.divider} />}


There might be a use case for embedded-view mode within the active visits that might necessitate this being updated.

I'm not sure of the use case but currently, the sidenav isn't supported in "embedded-view"

pirupius · 2024-11-01T15:08:25Z

src/components/sidebar/useCurrentActivePage.ts

+interface UseCurrentActivePageProps {
+  pages: FormPage[];
+  defaultPage: string;
+  activePages: string[];
+  evaluatedPagesVisibility: boolean;
+}
+
+interface UseCurrentActivePageResult {


Adding Use*** for interface naming makes the naming convention confusing. I think we should just change these 2 to CurrentActivePageProps and CurrentActivePageResult respectively especially since there not being exported anyways.

pirupius · 2024-11-01T15:35:54Z

src/form-engine.component.tsx

+      return false;
+    }
+
+    return ['extra-wide', 'ultra-wide'].includes(workspaceSize) && hasMultiplePages;


These modes are starting to get confusing. This implies 4 modes yet the design guidelines as well as workspace only has 3 modes. Does extra-wide have a different behavour from ultra-wide

ultra-wide is basically "full screen". See the screenshots in the PR.

Thanks @ibacher i saw that. Then i guess the issue would be with extra-wide unless that naming is being referenced else where within the patient chart.

pirupius · 2024-11-01T15:39:11Z

src/hooks/useFormWorkspaceSize.ts

+const narrowWorkspaceWidth = 26.25;
+const widerWorkspaceWidth = 32.25;
+const extraWideWorkspaceWidth = 48.25;
+
+type WorkspaceSize = 'narrow' | 'wider' | 'extra-wide' | 'ultra-wide';


This indicates that extra-wide and ultra-wide might be doing the same thing so maybe we should drop extra-wide naming

pirupius · 2024-11-01T15:42:23Z

src/hooks/useFormWorkspaceSize.ts

+  const size = useMemo(() => {
+    if (containerWidth <= narrowWorkspaceWidth) {
+      return 'narrow';
+    } else if (containerWidth <= widerWorkspaceWidth) {
+      return 'wider';
+    } else if (containerWidth <= extraWideWorkspaceWidth) {
+      return 'extra-wide';
+    } else {
+      return 'ultra-wide';
+    }


🤯 which dimensions will ultra-wide take up? or will it inherit from the screen size? This still feels like unnecessary.

ibacher

Nice work! A few nit-picking comments.

ibacher · 2024-11-01T16:07:16Z

src/components/sidebar/useCurrentActivePage.ts

+  useEffect(() => {
+    if (isInitialized || !evaluatedPagesVisibility) return;
+
+    const initializePage = () => {
+      // Try to find and set the default page
+      const defaultPageObject = pages.find(({ label }) => label === defaultPage);
+
+      if (defaultPageObject && !defaultPageObject.isHidden) {
+        setCurrentActivePage(defaultPageObject.id);
+        scrollIntoView(defaultPageObject.id);
+      } else {
+        // Fall back to first visible page
+        const firstVisiblePage = pages.find((page) => !page.isHidden);
+        if (firstVisiblePage) {
+          setCurrentActivePage(firstVisiblePage.id);
+        }
+      }
+    };
+
+    initializePage();
+    setIsInitialized(true);
+  }, [pages, defaultPage, evaluatedPagesVisibility, isInitialized]);


Why bother with the useEffect() here at all?:

if (!isInitialized && evaluatedPagesVisibility) { const defaultPageObject = pages.find(({ label }) => label === defaultPage); // blah, blah, blah }

This is actually also a weird case where using a ref instead of state to track whether or not the hook is initialized is actually a performance improvement because the side effect of calling setIsInitialized() is actually undesirable (we only want the re-render if setCurrentActivePage() is called).

I agree we can get rid of the useEffect().

This is actually also a weird case where using a ref instead of state to track whether or not the hook is initialized is actually a performance improvement because the side effect of calling setIsInitialized() is actually undesirable

There is a rationale for managing isInitialized as state. The logic that handles the Waypoint lockout needs to be triggered after initialization, hence the dependency.

Ah, yes. When I wrote this, I was hoping to get rid of most of the useEffect()s, but we definitely need it for the timeout

ibacher · 2024-11-01T16:09:00Z

src/components/sidebar/useCurrentActivePage.ts

+    // 1. Initial render completion
+    // 2. Scroll position establishment
+    // 3. Waypoint to complete its initial visibility detection
+    if (isInitialized) {


AFAICT, initialLockTimeoutRef is only used locally to this hook, so why not have it as a local?

ibacher · 2024-11-01T16:10:03Z

src/components/sidebar/useCurrentActivePage.ts

+    }
+
+    // Cleanup
+    return () => {


Suggested change

return () => {

return initialLockTimeoutRef.current ? () => {

// snip

}) : null;

ibacher · 2024-11-01T16:12:11Z

src/components/sidebar/useCurrentActivePage.ts

+      // If there's a requested page and it's visible, keep it active
+      if (requestedPage && activePages.includes(requestedPage)) {
+        setCurrentActivePage(requestedPage);
+        setTimeout(() => {


If we're creating a timeout, we should clear the timeout when the effect returns.

ibacher · 2024-11-01T16:12:23Z

src/components/sidebar/useCurrentActivePage.ts

+        setCurrentActivePage(requestedPage);
+        setTimeout(() => {
+          setRequestedPage(null);
+        }, 100);


Why 100ms? Could this just be 0?

This pushes the cleanup or clearing of the requested page to a later point. This ensures that we ignore all intermediate Waypoint events as we scroll to the requested page. So the 100ms delay is necessary.

ibacher · 2024-11-01T16:13:54Z

src/components/sidebar/useCurrentActivePage.ts

+  }, [isInitialized]);
+
+  // Handle active pages updates from viewport visibility
+  useEffect(() => {


Is there a way we can move this logic into the requestPage() callback itself?

It's possible but that may introduce some coupling or confusion because these events are propagated by disparate actions. A page request is made when the user clicks on a link in the sidenav while the other events are coming from Waypoint.

ibacher · 2024-11-01T16:17:05Z

src/components/sidebar/page-observer.ts

+import { BehaviorSubject } from 'rxjs';
+import { type FormPage } from '../../types';
+
+class PageObserver {


A couple of points here:

I don't love that this is a library-wide singleton rather than an object tied to a specific render tree, which is what it feels like it should be. This basically adds the artificial restriction that only one form instance (per instance of form-engine-lib) is active at a time.

Since we have a usePageObserver() hook, wouldn't it be better to confine all the reactivity to that? I'm not clear on the advantages of having a separate class here with a bunch of BehaviorSubjects.

Nice catch!

ibacher · 2024-11-01T16:19:03Z

src/components/sidebar/sidebar.component.tsx

+      {pages
+        .filter((page) => isPageContentVisible(page))
+        .map((page) => {
+          const isActive = page.id === currentActivePage;
+          const hasError = pagesWithErrors.includes(page.id);
+          return (
+            <div
+              className={classNames(styles.tab, {
+                [styles.activeTab]: isActive && !hasError,
+                [styles.errorTab]: hasError && !isActive,
+                [styles.activeErrorTab]: hasError && isActive,
+              })}>
+              <button
+                onClick={(e) => {
+                  e.preventDefault();
+                  requestPage(page.id);
+                }}>
+                <span>{page.label}</span>
+              </button>
+            </div>
+          );
+        })}


This feels like the type of thing that might be better handled in a useMemo() hook. At the least, the top-level divs here need a key property.

Better yet, we would refactor the div / button combo into its own component (it can be an internal component local to this file).

This feels like the type of thing that might be better handled in a useMemo() hook.

A more ideal approach would be memoizing this once in the form renderer and we expect the page observer to report only visible pages, but there is an issue:

Achieving this is likely to beg for the need of a state update of the entire form object every time a field changes, triggering the form to re-render every time the user interacts with a field. Currently, when a field's state changes, only the field and the flattened fields array state are updated (this doesn't trigger a re-render of the entire form).

But why would this be necessary? This boils down to how we determine the visibility of a page. A page is considered to have visible content if:

The page itself is not hidden.

At least one section within the page is visible.

At least one question within each section is visible.

I think for now, we can deploy keys to the heat-map level elements, leveraging React's reconciliation algorithm and address the bigger issue in a different PR.

Well, yeah, the whole need to re-render the form was kind of why I was suggesting local memoization, but I think I see your point: any field update can (potentially) trigger new fields to show and effect page visibility, and there isn't an obvious way to tie memoization to changing page visibility, so let's leave that aside.

ibacher · 2024-11-01T16:20:26Z

src/components/sidebar/sidebar.scss

-}
-
-.section {
+.tab {


I think .tab is a pretty bad name here. Surely "page" or "pageLink" is better?

ibacher · 2024-11-07T14:34:50Z

src/hooks/useFormWorkspaceSize.ts

+  useLayoutEffect(() => {
+    const handleResize = () => {
+      const containerWidth = rootRef.current?.parentElement?.offsetWidth;
+      containerWidth && setContainerWidth(pxToRem(containerWidth));


So, for this to be done correct, we need to always pass the actual root element font size:

const rootFontSize = parseInt(getComputedStyle(document.documentElement).fontSize);

Nice catch!

ibacher · 2024-11-07T14:35:10Z

src/hooks/useFormWorkspaceSize.ts

+      handleResize();
+    });
+
+    if (rootRef.current) {


Shouldn't the check here actually be:

Suggested change

if (rootRef.current) {

if (rootRef.current?.parentElement) {

samuelmale · 2024-11-19T10:18:52Z

@ibacher kindly review my latest changes so that we can have this go in.

samuelmale marked this pull request as draft October 24, 2024 12:30

samuelmale force-pushed the feat/restoreSidenav branch 2 times, most recently from 2861780 to b8edaaa Compare October 29, 2024 01:50

samuelmale marked this pull request as ready for review October 29, 2024 12:01

samuelmale requested review from denniskigen, brandones, ibacher and pirupius October 29, 2024 12:02

pirupius reviewed Nov 1, 2024

View reviewed changes

ibacher reviewed Nov 1, 2024

View reviewed changes

ibacher reviewed Nov 7, 2024

View reviewed changes

samuelmale force-pushed the feat/restoreSidenav branch from 257a987 to 9d8e2cb Compare November 14, 2024 12:50

samuelmale added 5 commits November 19, 2024 21:24

Restore sidenav

cfc011b

Simplify logic and add tests

f797768

Show inline loader when submitting

4aecbd9

fixup

08d3828

Revise CSS class names

1c17dc8

samuelmale force-pushed the feat/restoreSidenav branch from b238831 to 1c17dc8 Compare November 19, 2024 18:26

denniskigen requested a review from ibacher November 21, 2024 13:16

ibacher approved these changes Nov 22, 2024

View reviewed changes

brandones merged commit e4d6204 into main Nov 22, 2024
4 checks passed

brandones deleted the feat/restoreSidenav branch November 22, 2024 23:50

-    return () => {
+    return initialLockTimeoutRef.current ? () => {
+      // snip
+    }) : null;

(feat) Restore side navigation #417

(feat) Restore side navigation #417

Conversation

samuelmale commented Oct 24, 2024 • edited Loading

Requirements

Summary

Under the Hood

Known Limitations

Screenshots

Related Issue

Other

github-actions bot commented Oct 24, 2024 • edited Loading

denniskigen commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher left a comment

Choose a reason for hiding this comment

ibacher Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelmale Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelmale commented Nov 19, 2024

samuelmale commented Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024 •

edited

Loading

ibacher Nov 1, 2024 •

edited

Loading

samuelmale Nov 7, 2024 •

edited

Loading