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

Fixes issue with set anchor method #3761

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

walmazacn
Copy link
Contributor

Description

Changes proposed in this pull request:

  • fixes issue with set anchor method for WC MF in modal

Related issue(s)

Resolves #3399

@walmazacn walmazacn linked an issue May 29, 2024 that may be closed by this pull request
@ndricimrr ndricimrr self-assigned this May 31, 2024
@ndricimrr
Copy link
Contributor

I checked the issue and it seems like this originated from this comment which helped reproduce the issue.

From the commit log that followed thereafter that comment it seems like it was decided that we just do not support setAnchor functionality for modals (and possibly other special microfrontends like splitview, drawer etc) at the moment as further investigation on how to do it is needed.

We have introduced the isSpecialMF predicate for this here. In such a case if you follow the reproduce steps, the setAnchor function call is simply not reached because we disabled it.

So I think the changes on this PR do not address that currently, and will need to change the issue to be a feature request than a bug. Discuss it in daily

Copy link
Contributor

@ndricimrr ndricimrr left a comment

Choose a reason for hiding this comment

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

Tested some edge cases and found some potential scenario where this could break

core/src/App.svelte Outdated Show resolved Hide resolved
core/src/App.svelte Outdated Show resolved Hide resolved
core/src/App.svelte Outdated Show resolved Hide resolved
for (let i = mfModalList.length; i--; ) {
closeModal(i);

if (!mfModalList.some((item) => item.modalWC && item.mfModal?.displayed)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition is a bit hard to read directly. If I can read it correctly this means that:

"if in the list of modals there is at least an item that is WC-based and is currently at the top of the stack=displayed"
         don't do anything
else 
        close all modals

Maybe the developer had another scenario where they have WC Based wc and displayed and want to close the modals also, not 'do nothing'.

I feel like in this way, can always find a failing scenario. For example, now, this fails to close all modals:

Luigi.navigation().openAsModal('/test/modalwc', { size:'l', keepPrevious: true});
Luigi.navigation().openAsModal('/test/modalwc2', { size:'m', keepPrevious: true});
Luigi.navigation().openAsModal('/test/overview', { size:'s', keepPrevious: true});
// navigate away
Luigi.navigation().navigate('/test/overview');

Question is though, instead of solving scenario after scenario, maybe rethink, what do we want to do when setAnchor is called inside a modal WC.
Maybe closing all modals and leaving the anchor at the main navigation could also be valid. I am not sure, I'll ask in daily again, since its not a common request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After my latest fix the scenario above will not fail, as all modals are closed after navigating away (separate process). The condition you're mentioning happens after route is updated, so it won't break anything as all modals are closed already

@walmazacn walmazacn marked this pull request as draft June 13, 2024 12:17
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.

setAnchor for wc mf in modal
2 participants