Bookmarkable modals

[maxmarkus]

Changes proposed in this pull request:

• Persistence of modal path and parameters in the url
• Configurable parameter name: routing.showModalPathInUrl and routing.modalPathParam
• nodeParams are working

TODO in this draft:

• unit tests

How to reproduce:

• run npm run simpledev in core
• run npm start in test/e2e-testapplication
• copy luigi-config:

core/dev-tools/simple-app/luigi-config.js

Luigi.setConfig({
  navigation: {
    nodes: [{
      pathSegment: 'home',
      hideFromNav: true,
      // hideSideNav: true,
      loadingIndicator: {
        enabled: false
      },
      viewUrl: '/microfrontend.html',
      children: [
        {
          label: 'Child 1',
          pathSegment: 'child',
          loadingIndicator: {
            enabled: false
          },
          viewUrl: 'http://127.0.0.1:8090/#child-1'
        },
        {
          label: 'Child 2',
          pathSegment: 'child-2',
          loadingIndicator: {
            enabled: false
          },
          viewUrl: '/microfrontend.html',
          // openNodeInModal: {
          //   title: 'Real Child'
          // }
        }
      ]
    }]
  },
  routing: {
    showModalPathInUrl: true,
    modalPathParam: 'modal'
  },
  settings: {
    responsiveNavigation: 'Fiori3',
    header: {
      title: 'Luigi Simple Dev'
    },
    theming: {
      themes: [
        { id: 1, name: 'light' },
        { id: 2, name: 'dark' },
      ],
      defaultTheme: 'dark'
    }
  }
});

http://localhost:4100/home/child-2?modal=%252Fhome%252Fchild%253F~foo%253Dbar&modalParams=%257B%2522title%2522%253A%2522Hello%2520Luigi%2522%257D

Related issue(s)
Resolves #1772

[JohannesDoberer]

Missing test should not covered by the review

[legteodav]

Shouldn't you check if the param is present in location.search? like...

if (!prevModalPath){
    return;
}

[maxmarkus]

In this case there can or cannot be an existing modal. If we would return here, we would never append to the url.

[legteodav]

async handleRouteChange(path, component, iframeElement, config, withoutSync) function is getting pretty long and complex (it would be nice to refactor).
Could you create a new function:

async handleBookmarkableModalPath(){
    const additionalModalPath = RoutingHelpers.getModalPathFromPath();
    if (additionalModalPath) {
      const modalParams = RoutingHelpers.getModalParamsFromPath();
      const { nodeObject } = await Navigation.extractDataFromPath(additionalModalPath);
      LuigiNavigation.openAsModal(additionalModalPath, nodeObject.openNodeInModal || modalParams);
    }
  }

Suggested change

	// handle bookmarkable modal path
	const additionalModalPath = RoutingHelpers.getModalPathFromPath();
	if (additionalModalPath) {
	const modalParams = RoutingHelpers.getModalParamsFromPath();
	const { nodeObject } = await Navigation.extractDataFromPath(additionalModalPath);
	LuigiNavigation.openAsModal(additionalModalPath, nodeObject.openNodeInModal || modalParams);
	}
	await this.handleBookmarkableModalPath();

[maxmarkus]

Yep, I have not taken time to refactor yet, wanted to get the things working first. I appreciate your thoughts 👍

[maxmarkus]

I have updated the e2e test cases and fixed the implementation. One thing to mention: I have implemented a flexible "getQueryParam" and "getQueryParams" method in RoutingHelpers, since GenericHelpers.getUrlParams did not work properly. It might be an opportunity to consolidate other occurrences if this fits the same use case. Happy to have a talk about it too.

Added showModalPathInUrl: true, to the example above.

Unit tests will come in the next days.

[JohannesDoberer]

Summery of the call with Markus:
Code looks good from my side.
Markus will provide the missing unit tests.

There is one tiny issue. When you open a modal from the left nav the url will not be generated. Markus will take a look and either it is a quick fix he will fix it in this PR or if not we will create a follow up ticket for that.

[JohannesDoberer]

Good job!!Looks good to me

[ndricimrr]

I see an error when attempting with the following configuration:

showModalPathInUrl = true;
 modalPathParam = 'modal';

and then for a particular node:
openNodeInModal: true,

Screenshot:

It works well with:
openNodeInModal: {title: "test"}

It just seems that for true value it still expects to find a title.

[ndricimrr]

Looks good 👍