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

Jump to bottom

Fixes issue with set anchor method #3761

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

Fixes issue with set anchor method #3761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
2d5cb7f
Fixes issue with set anchor method
walmazacn May 29, 2024
8f31d7d
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn May 31, 2024
562d076
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jun 4, 2024
50bd30d
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jun 4, 2024
0558fde
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jun 6, 2024
b4090b3
Fixes code review issue
walmazacn Jun 6, 2024
75db56b
Merge branch '3399-set-anchor-for-wc-mf-in-modal' of https://github.c…
walmazacn Jun 6, 2024
5aa4fbf
Fixes code review issues
walmazacn Jun 7, 2024
60567d5
Fixes minor issue
walmazacn Jun 7, 2024
7c6ffec
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jun 7, 2024
bc2ac37
Fixes code review issue
walmazacn Jun 7, 2024
ce23d31
Merge branch '3399-set-anchor-for-wc-mf-in-modal' of https://github.c…
walmazacn Jun 7, 2024
0a4765a
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jun 7, 2024
d469445
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jun 10, 2024
b15835c
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jun 13, 2024
e9a6361
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jun 19, 2024
f3cd9df
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jun 21, 2024
02f7235
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jul 2, 2024
0190ee6
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jul 16, 2024
7dcbc4b
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jul 17, 2024
a970aaa
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Jul 29, 2024
38a6999
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Aug 2, 2024
da76e5a
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Aug 6, 2024
766d5fe
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Aug 29, 2024
2648857
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Sep 9, 2024
71680a4
Merge branch 'main' into 3399-set-anchor-for-wc-mf-in-modal
walmazacn Sep 13, 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
20 changes: 14 additions & 6 deletions core/src/App.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,13 +375,18 @@
RoutingHelpers.addRouteChangeListener((path, eventDetail) => {
const { withoutSync, preventContextUpdate } = eventDetail || {};
const pv = preservedViews;

// TODO: check if bookmarkable modal is interferring here
if (!isValidBackRoute(pv, path)) {
preservedViews = [];
Iframe.removeInactiveIframes(node);
}
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

// close all modals as we are navigating away here
for (let i = mfModalList.length; i--; ) {
closeModal(i);
}
}

// remove backdrop
Expand Down Expand Up @@ -933,6 +938,7 @@
};
const targetModal = mfModalList[index];
const rp = GenericHelpers.getRemotePromise(targetModal.mfModal.settings.onClosePromiseId);

if (targetModal && targetModal.modalIframe) {
getUnsavedChangesModalPromise(targetModal.modalIframe.contentWindow).then(
() => {
Expand Down Expand Up @@ -1405,10 +1411,12 @@
rejectRemotePromise();
});
// close all modals to allow navigation to the non-special view
mfModalList.forEach((m, index) => {
// close modals
closeModal(index);
});
let mfModalListLength = mfModalList.length;
while (mfModalListLength) {
mfModalListLength--;
// close modal
closeModal(mfModalListLength);
}

closeSplitView();
closeDrawer();
Expand Down
4 changes: 1 addition & 3 deletions core/src/services/web-components.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,7 @@ class WebComponentSvcClass {
return wc.extendedContext.nodeParams;
},
setAnchor: anchor => {
if (!isSpecialMf) {
window.Luigi.routing().setAnchor(anchor);
}
window.Luigi.routing().setAnchor(anchor);
},
getAnchor: () => {
return window.Luigi.routing().getAnchor();
Expand Down