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

UI AppShell containing the whole plugin #1548

Closed
ecgan opened this issue Jun 2, 2022 · 2 comments
Closed

UI AppShell containing the whole plugin #1548

ecgan opened this issue Jun 2, 2022 · 2 comments
Labels
type: enhancement The issue is a request for an enhancement. type: technical debt This issue/PR represents/solves the technical debt of the project.

Comments

@ecgan
Copy link
Member

ecgan commented Jun 2, 2022

User story

This is a proposal and a follow-up to #1543 (review):

For a long term better solution, I think we should have an AppShell that contains the whole plugin, and things like the CustomerEffortScorePrompt, DifferentCurrencyNotice and NavigationClassic components should be in the AppShell, so that there will only be one instance of them throughout the whole app. With that approach, we wouldn't need to have this useEffect call. This approach would also solve the issue where the notice appears to keep re-appearing when we switch tabs, see demo video of the issue below:

Here are a few problems that we have in our existing code without AppShell:

Describe the solution you'd like

Have an AppShell that contains the whole plugin, so that the above mentioned components can be placed in the AppShell, thus addressing the problems.

@ecgan ecgan added type: enhancement The issue is a request for an enhancement. type: technical debt This issue/PR represents/solves the technical debt of the project. labels Jun 2, 2022
@tomalec
Copy link
Member

tomalec commented Jun 3, 2022

💡 (probably side-tracking) Having the AppShell reminds me of the concept of app shell from PWAs. I think it would be good to have some (preferably service worker) infrastructure from WP-admin/WC to the app shell. A one that's heavily cached in user's browser, and available offline.

@eason9487
Copy link
Member

eason9487 commented Aug 30, 2023

Here are a few problems that we have in our existing code without AppShell:

  • [...]
  • We have multiple instances of the tab navigation component in multiple components. Because it is not a single tab component, we lose the fancy tab switch animation.
  • [...]

I tried tweaking NavigationClassic, which is the most used on this extension's page. In the ways I've experimented, moving it into withAdminPageShell requires an additional layer of abstraction or a certain level of indirect conditions to control whether or not to display it.

In comparison, I think the intuitive use of a <NavigationClassic /> on the needed pages is preferable to the added complexity of these tweaks.


I'm going to close this issue since the conceptual equivalent of AppShell has been added in #1739.
See withAdminPageShell

If there is a need to implement the example(s) mentioned in this issue in the future, we can open a separate issue then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement The issue is a request for an enhancement. type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
Development

No branches or pull requests

3 participants