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.

Sign up for GitHub

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

FIO-9241: Correctly set current page after conditionally hiding page in sibling nested wizard #5913

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

FIO-9241: Correctly set current page after conditionally hiding page in sibling nested wizard #5913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a93889
FIO-9241 Wizard: Rename hasExtraPages getter
blakekrammes Nov 20, 2024
efabe18
FIO-9241 Wizard: Fix bug in render with dup keys
blakekrammes Nov 20, 2024
d415fcc
FIO-9241 Wizard: Set page after conditional eval in nested wizard
blakekrammes Nov 20, 2024
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
41 changes: 17 additions & 24 deletions src/Wizard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ export default class Wizard extends Webform {
this.originalComponents = [];
this.page = 0;
this.currentPanel = null;
this.currentPanels = null;
this.currentNextPage = 0;
this._seenPages = [0];
this.subWizards = [];
Expand All @@ -60,14 +59,14 @@ export default class Wizard extends Webform {

getPages(args = {}) {
const { all = false } = args;
const pages = this.hasExtraPages ? this.components : this.pages;
const pages = this.hasSubWizards ? this.components : this.pages;
const filteredPages = pages
.filter(all ? _.identity : (p, index) => this._seenPages.includes(index));

return filteredPages;
}

get hasExtraPages() {
get hasSubWizards() {
return !_.isEmpty(this.subWizards);
}

Expand Down Expand Up @@ -216,9 +215,9 @@ export default class Wizard extends Webform {
render() {
const ctx = this.renderContext;

if (this.component.key) {
ctx.panels.map(panel => {
if (panel.key === this.component.key) {
if (this.component.id) {
Copy link
Contributor Author

@blakekrammes blakekrammes Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There was an additional bug here:

If you had multiple/sibling nested wizards and conditional logic controlling the visibility of a nested wizard page, this was setting the currentPanel to the wrong page on render (i.e. both forms have a 'page1' key and the wizard would navigate to 'page1' on nested form C instead of 'page1' on nested form B. This is fixed by using a unique id to set the currentPanel.

ctx.panels.forEach(panel => {
Copy link
Contributor Author

@blakekrammes blakekrammes Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

.forEach is more appropriate since we're applying side-effects and not returning a new array.

if (panel.id === this.component.id) {
this.currentPanel = panel;
ctx.wizardPageTooltip = this.getFormattedTooltip(panel.tooltip);
}
Expand Down Expand Up @@ -676,7 +675,7 @@ export default class Wizard extends Webform {
this.getNextPage();

let parentNum = num;
if (this.hasExtraPages) {
if (this.hasSubWizards) {
const pageFromPages = this.pages[num];
const pageFromComponents = this.components[num];
if (!pageFromComponents || pageFromPages?.id !== pageFromComponents.id) {
Expand Down Expand Up @@ -1006,24 +1005,18 @@ export default class Wizard extends Webform {
}

// If the pages change, need to redraw the header.
let currentPanels;
let panels;
const currentPanels = this.pages;
// calling this.establishPages() updates/mutates this.pages to be the current pages
this.establishPages();
const newPanels = this.pages;
const currentNextPage = this.currentNextPage;
if (this.hasExtraPages) {
currentPanels = this.pages.map(page => page.component.key);
this.establishPages();
panels = this.pages.map(page => page.component.key);
const panelsUpdated = !_.isEqual(newPanels, currentPanels);

if (this.currentPanel?.id && this.pages.length && (!this.hasSubWizards || (this.hasSubWizards && panelsUpdated))) {
const newIndex = this.pages.findIndex(page => page.id === this.currentPanel.id);
if (newIndex !== -1) this.setPage(newIndex);
}
else {
currentPanels = this.currentPanels || this.pages.map(page => page.component.key);
Copy link
Contributor Author

@blakekrammes blakekrammes Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this.currentPanels was only referenced within onChange and seems like an unnecessary piece of state given that the current pages/panels can be accessed via this.pages. I could be missing/unaware of a case, but all the tests pass happily without it and I really wanted to simplify this logic.

From what I can tell, this.currentPanels was only tracking the parent pages, which in the case of a wizard having nested wizards forms A, B and C, would only return the top-level parent pages for those forms. However, that's not useful when you're trying to update the current page to a particular page in one of those nested forms. I don't believe it's possible to navigate to a parent page A without expanding its children and selecting the first page A1. this.pages is a flattened array and stores every page in every nested wizard and can be used to select the correct page after onChange is called for both simple, single-level wizards, and wizards with nested wizards.

I think the reason why this.currentPanels worked for selecting the current page in single-level wizards was because the top-level panel components are navigable pages. However, when you have nested wizards, they are not navigable pages.

For comparison:

this.currentPanels = [{ title: 'A', ... }, { title: 'B', ... }, { title: 'C', ... }]
this.pages = [{ title: 'A1', ... }, { title: 'A2', ... },  { title: 'A3', ... }, { title: 'B1', ... }, ...]

panels = this.establishPages().map(panel => panel.key);
this.currentPanels = panels;
if (this.currentPanel?.key && this.currentPanels?.length) {
this.setPage(this.currentPanels.findIndex(panel => panel === this.currentPanel.key));
}
}

if (!_.isEqual(panels, currentPanels) || (flags && flags.fromSubmission)) {
if (panelsUpdated || (flags && flags.fromSubmission)) {
this.redrawHeader();
}

Expand Down Expand Up @@ -1074,7 +1067,7 @@ export default class Wizard extends Webform {
}

showErrors(errors, triggerEvent) {
if (this.hasExtraPages) {
if (this.hasSubWizards) {
this.subWizards.forEach((subWizard) => {
if(Array.isArray(subWizard.errors)) {
errors = [...errors, ...subWizard.errors]
Expand Down
2 changes: 1 addition & 1 deletion src/components/_classes/nested/NestedComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ export default class NestedComponent extends Field {

validationProcessor({ scope, data, row, instance, component }, flags) {
const { dirty } = flags;
if (this.root.hasExtraPages && this.page !== this.root.page) {
if (this.root.hasSubWizards && this.page !== this.root.page) {
instance = this.childComponentsMap?.hasOwnProperty(component.path)
? this.childComponentsMap[component.path]
: this.getComponent(component.path);
Expand Down
Loading
Loading