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 side navigation #417

Merged
merged 5 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/components/renderer/form/form-renderer.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import { FormProvider, type FormContextProps } from '../../../provider/form-prov
import { isTrue } from '../../../utils/boolean-utils';
import { type FormProcessorContextProps } from '../../../types';
import { useFormStateHelpers } from '../../../hooks/useFormStateHelpers';
import { pageObserver } from '../../sidebar/page-observer';
import { isPageContentVisible } from '../../../utils/form-helper';

export type FormRendererProps = {
processorContext: FormProcessorContextProps;
Expand All @@ -23,7 +25,10 @@ export const FormRenderer = ({
isSubForm,
setIsLoadingFormDependencies,
}: FormRendererProps) => {
const { evaluatedFields, evaluatedFormJson } = useEvaluateFormFieldExpressions(initialValues, processorContext);
const { evaluatedFields, evaluatedFormJson, evaluatedPagesVisibility } = useEvaluateFormFieldExpressions(
initialValues,
processorContext,
);
const { registerForm, setIsFormDirty, workspaceLayout, isFormExpanded } = useFormFactory();
const methods = useForm({
defaultValues: initialValues,
Expand All @@ -50,6 +55,19 @@ export const FormRenderer = ({
setForm,
} = useFormStateHelpers(dispatch, formFields);

useEffect(() => {
const scrollablePages = formJson.pages.filter((page) => !page.isSubform).map((page) => page);
pageObserver.updateScrollablePages(scrollablePages);
}, [formJson.pages]);
Comment on lines +58 to +61
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't changing this to a useMemo be a better option for 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.

I think the useEffect is ideal for this scenario.


useEffect(() => {
pageObserver.setEvaluatedPagesVisibility(evaluatedPagesVisibility);
}, [evaluatedPagesVisibility]);

useEffect(() => {
pageObserver.updatePagesWithErrors(invalidFields.map((field) => field.meta.pageId));
}, [invalidFields]);

const context: FormContextProps = useMemo(() => {
return {
...processorContext,
Expand Down Expand Up @@ -80,11 +98,7 @@ export const FormRenderer = ({
return (
<FormProvider {...context}>
{formJson.pages.map((page) => {
const pageHasNoVisibleContent =
page.sections?.every((section) => section.isHidden) ||
page.sections?.every((section) => section.questions?.every((question) => question.isHidden)) ||
isTrue(page.isHidden);
if (!page.isSubform && pageHasNoVisibleContent) {
if (!page.isSubform && !isPageContentVisible(page)) {
return null;
}
if (page.isSubform && page.subform?.form) {
Expand Down
17 changes: 12 additions & 5 deletions src/components/renderer/page/page.renderer.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ import { SectionRenderer } from '../section/section-renderer.component';
import { Waypoint } from 'react-waypoint';
import styles from './page.renderer.scss';
import { Accordion, AccordionItem } from '@carbon/react';
import { useFormFactory } from '../../../provider/form-factory-provider';
import { ChevronDownIcon, ChevronUpIcon } from '@openmrs/esm-framework';
import classNames from 'classnames';
import { pageObserver } from '../../sidebar/page-observer';

interface PageRendererProps {
page: FormPage;
Expand All @@ -24,10 +24,8 @@ interface CollapsibleSectionContainerProps {

function PageRenderer({ page, isFormExpanded }: PageRendererProps) {
const { t } = useTranslation();
const pageId = useMemo(() => page.label.replace(/\s/g, ''), [page.label]);
const [isCollapsed, setIsCollapsed] = useState(false);

const { setCurrentPage } = useFormFactory();
const visibleSections = useMemo(
() =>
page.sections.filter((section) => {
Expand All @@ -41,12 +39,21 @@ function PageRenderer({ page, isFormExpanded }: PageRendererProps) {

useEffect(() => {
setIsCollapsed(!isFormExpanded);

return () => {
pageObserver.removeInactivePage(page.id);
};
Comment on lines +43 to +45
Copy link
Member

Choose a reason for hiding this comment

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

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

}, [isFormExpanded]);

return (
<div>
<Waypoint onEnter={() => setCurrentPage(pageId)} topOffset="50%" bottomOffset="60%">
<div className={styles.pageContent}>
<Waypoint
key={page.id}
onEnter={() => pageObserver.addActivePage(page.id)}
onLeave={() => pageObserver.removeInactivePage(page.id)}
topOffset="40%"
bottomOffset="40%">
<div id={page.id} className={styles.pageContent}>
<div className={styles.pageHeader} onClick={toggleCollapse}>
<p className={styles.pageTitle}>
{t(page.label)}
Expand Down
58 changes: 58 additions & 0 deletions src/components/sidebar/page-observer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { BehaviorSubject } from 'rxjs';
import { type FormPage } from '../../types';

class PageObserver {
Copy link
Member

Choose a reason for hiding this comment

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

A couple of points here:

  1. 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.
  2. 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch!

private scrollablePagesSubject = new BehaviorSubject<Array<FormPage>>([]);
private pagesWithErrorsSubject = new BehaviorSubject<Set<string>>(new Set());
private activePagesSubject = new BehaviorSubject<Set<string>>(new Set());
private evaluatedPagesVisibilitySubject = new BehaviorSubject<boolean>(null);

setEvaluatedPagesVisibility(evaluatedPagesVisibility: boolean) {
this.evaluatedPagesVisibilitySubject.next(evaluatedPagesVisibility);
}

updateScrollablePages(newPages: Array<FormPage>) {
this.scrollablePagesSubject.next(newPages);
}

updatePagesWithErrors(newErrors: string[]) {
this.pagesWithErrorsSubject.next(new Set(newErrors));
}

addActivePage(pageId: string) {
const currentActivePages = this.activePagesSubject.value;
currentActivePages.add(pageId);
this.activePagesSubject.next(currentActivePages);
}

removeInactivePage(pageId: string) {
const currentActivePages = this.activePagesSubject.value;
currentActivePages.delete(pageId);
this.activePagesSubject.next(currentActivePages);
}

getActivePagesObservable() {
return this.activePagesSubject.asObservable();
}

getScrollablePagesObservable() {
return this.scrollablePagesSubject.asObservable();
}

getPagesWithErrorsObservable() {
return this.pagesWithErrorsSubject.asObservable();
}

getEvaluatedPagesVisibilityObservable() {
return this.evaluatedPagesVisibilitySubject.asObservable();
}

clear() {
this.scrollablePagesSubject.next([]);
this.pagesWithErrorsSubject.next(new Set());
this.activePagesSubject.next(new Set());
this.evaluatedPagesVisibilitySubject.next(false);
}
}

export const pageObserver = new PageObserver();
162 changes: 68 additions & 94 deletions src/components/sidebar/sidebar.component.tsx
Original file line number Diff line number Diff line change
@@ -1,134 +1,108 @@
import React, { useCallback, useEffect, useMemo } from 'react';
import React from 'react';
import classNames from 'classnames';
import { useTranslation } from 'react-i18next';
import { Button, Toggle } from '@carbon/react';
import { isEmpty } from '../../validators/form-validator';
import { type FormPage } from '../../types';
import { Button } from '@carbon/react';
import { type FormPage, type SessionMode } from '../../types';
import styles from './sidebar.scss';
import { scrollIntoView } from '../../utils/form-helper';
import { usePageObserver } from './usePageObserver';
import { useCurrentActivePage } from './useCurrentActivePage';
import { isPageContentVisible } from '../../utils/form-helper';
import { InlineLoading } from '@carbon/react';

interface SidebarProps {
allowUnspecifiedAll: boolean;
defaultPage: string;
handleClose: () => void;
hideFormCollapseToggle: () => void;
isFormSubmitting: boolean;
mode: string;
sessionMode: SessionMode;
onCancel: () => void;
pagesWithErrors: string[];
scrollablePages: Set<FormPage>;
selectedPage: string;
setValues: (values: unknown) => void;
values: object;
handleClose: () => void;
hideFormCollapseToggle: () => void;
}

const Sidebar: React.FC<SidebarProps> = ({
allowUnspecifiedAll,
defaultPage,
handleClose,
hideFormCollapseToggle,
isFormSubmitting,
mode,
sessionMode,
onCancel,
pagesWithErrors,
scrollablePages,
selectedPage,
setValues,
values,
handleClose,
hideFormCollapseToggle,
}) => {
const { t } = useTranslation();
const pages: Array<FormPage> = Array.from(scrollablePages);

useEffect(() => {
if (defaultPage && pages.some(({ label, isHidden }) => label === defaultPage && !isHidden)) {
scrollIntoView(joinWord(defaultPage));
}
}, [defaultPage, scrollablePages]);

const unspecifiedFields = useMemo(
() =>
Object.keys(values).filter(
(key) => key.endsWith('-unspecified') && isEmpty(values[key.split('-unspecified')[0]]),
),
[values],
);

const handleClick = (selected) => {
const activeId = joinWord(selected);
scrollIntoView(activeId);
};

const markAllAsUnspecified = useCallback(
(toggled) => {
const updatedValues = { ...values };
unspecifiedFields.forEach((field) => {
updatedValues[field] = toggled;
});
setValues(updatedValues);
},
[unspecifiedFields, values, setValues],
);
const { pages, pagesWithErrors, activePages, evaluatedPagesVisibility } = usePageObserver();
const { currentActivePage, requestPage } = useCurrentActivePage({
pages,
defaultPage,
activePages,
evaluatedPagesVisibility,
});

return (
<div className={styles.sidebar}>
{pages.map((page, index) => {
if (page.isHidden) return null;
{pages
.filter((page) => isPageContentVisible(page))
.map((page) => (
<PageLink
key={page.id}
page={page}
currentActivePage={currentActivePage}
pagesWithErrors={pagesWithErrors}
requestPage={requestPage}
/>
))}
{sessionMode !== 'view' && <hr className={styles.divider} />}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


const isCurrentlySelected = joinWord(page.label) === selectedPage;
const hasError = pagesWithErrors.includes(page.label);

return (
<div
aria-hidden="true"
className={classNames({
[styles.erroredSection]: isCurrentlySelected && hasError,
[styles.activeSection]: isCurrentlySelected && !hasError,
[styles.activeErroredSection]: !isCurrentlySelected && hasError,
[styles.section]: !isCurrentlySelected && !hasError,
})}
key={index}
onClick={() => handleClick(page.label)}>
<div className={styles.sectionLink}>{page.label}</div>
</div>
);
})}
{mode !== 'view' && <hr className={styles.divider} />}
<div className={styles.sidenavActions}>
{allowUnspecifiedAll && mode !== 'view' && (
<div className={styles.toggleContainer}>
<Toggle
id="auto-unspecifier"
labelA={t('unspecifyAll', 'Unspecify All')}
labelB={t('revert', 'Revert')}
labelText=""
onToggle={markAllAsUnspecified}
/>
</div>
)}
{mode !== 'view' && (
<div className={styles.sideNavActions}>
{sessionMode !== 'view' && (
<Button className={styles.saveButton} disabled={isFormSubmitting} type="submit">
{t('save', 'Save')}
{isFormSubmitting ? (
<InlineLoading description={t('submitting', 'Submitting') + '...'} />
) : (
<span>{`${t('save', 'Save')}`}</span>
)}
</Button>
)}
<Button
className={classNames(styles.saveButton, {
[styles.topMargin]: mode === 'view',
className={classNames(styles.closeButton, {
[styles.topMargin]: sessionMode === 'view',
})}
kind="tertiary"
onClick={() => {
onCancel?.();
handleClose?.();
hideFormCollapseToggle();
}}>
{mode === 'view' ? t('close', 'Close') : t('cancel', 'Cancel')}
{sessionMode === 'view' ? t('close', 'Close') : t('cancel', 'Cancel')}
</Button>
</div>
</div>
);
};

function joinWord(value) {
return value.replace(/\s/g, '');
interface PageLinkProps {
page: FormPage;
currentActivePage: string;
pagesWithErrors: string[];
requestPage: (page: string) => void;
}

function PageLink({ page, currentActivePage, pagesWithErrors, requestPage }: PageLinkProps) {
const isActive = page.id === currentActivePage;
const hasError = pagesWithErrors.includes(page.id);
return (
<div
className={classNames(styles.pageLink, {
[styles.activePage]: isActive && !hasError,
[styles.errorPage]: hasError && !isActive,
[styles.activeErrorPage]: hasError && isActive,
})}>
<button
onClick={(e) => {
e.preventDefault();
requestPage(page.id);
}}>
<span>{page.label}</span>
</button>
</div>
);
}

export default Sidebar;
Loading