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

SceneAppPage: preserving tabs #567

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

xforman2
Copy link
Contributor

@xforman2 xforman2 commented Feb 5, 2024

So after discovering #559, I thought it would be nice to have a option to preserve the tabs.
I came up with an idea.

Now the SceneAppPageRenderer always renders the first tab with two urls: the parent page url and the actual tab url.
But if we can remember the last selected tab, we can match the parent page url to any tab. This is what _lastTabIndex is for.
And you also have an option to choose not preserving tabs with preserveTab property.

Let me know what you think.

@xforman2 xforman2 changed the title implemented preserving tabs logic SceneAppPage: preserving tabs Feb 5, 2024
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Not sure this makes sense, tabs have urls. so if you want to change the default route/tab for page you will need to do that through some redirect.

@xforman2
Copy link
Contributor Author

xforman2 commented Feb 5, 2024

I do not want to change the default route, I just try to change what will be rendered. So I just reused the old logic:

  • The 1st tab in the array of tabs is being rendered with the default url (parent page).

And now:

  • The parent SceneAppPage remembers the last active tab and in SceneAppPageRenderer adds the last active tab to render with a default route (parent page url).

Hope that makes it more clear.

@torkelo
Copy link
Member

torkelo commented Feb 6, 2024

that seems a bit wrong that different pages are rendered for the same url

@xforman2
Copy link
Contributor Author

xforman2 commented Feb 7, 2024

I agree that it is not good to have different pages render with the same url, so I tried what you suggested in the first answer. What do you think?

@xforman2
Copy link
Contributor Author

@torkelo I am not sure if you looked at my other implementation but it would be really helpful if this could get resolved

@torkelo
Copy link
Member

torkelo commented Feb 28, 2024

I have, the solution looks a bit complex / messy. Should be a simpler way to do this

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.

2 participants