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

New CoreBase #9102

Closed
4 tasks done
nucleogenesis opened this issue Feb 13, 2022 · 5 comments · Fixed by #9128
Closed
4 tasks done

New CoreBase #9102

nucleogenesis opened this issue Feb 13, 2022 · 5 comments · Fixed by #9128
Assignees
Labels
P0 - critical Priority: Release blocker or regression

Comments

@nucleogenesis
Copy link
Member

nucleogenesis commented Feb 13, 2022

Context

EDIT: No more borrowing PageRoot - let's make a new component altogether.

In order to begin updating Kolibri's many plugins and uses of CoreBase, we must establish a new component built to our new ideals that we can start implementing without regressions.

My suggestion is to take create a new component for this purpose. This will give us a component that accommodates the following suggestions:

  • The GlobalSnackbar component should be loaded into every page. This means that the GlobalSnackbar can show itself anytime it sees the correct Vuex state.
  • The Core Vuex store's error state property is universally how we will decide if the user should see one of either an AuthMessage error message or a general AppError message.
  • The UpdateNotification component should be available on every page (and, naturally, rendered under the correct circumstances)
  • Renders, otherwise, only a default slot, into which any implementing page in the app can display what they need to.

Acceptance Criteria

  • Create a new root component, include it in the apiSpec
  • Move the four aforementioned components to the new component. The error components should follow a similar logical flow as in CoreBase. The other two components need be loaded into the app.
  • The component should render the default slot when there are no errors.
  • Unit tests covering conditions for showing error messages vs router view
@nucleogenesis nucleogenesis added the P0 - critical Priority: Release blocker or regression label Feb 13, 2022
@MisRob
Copy link
Member

MisRob commented Feb 14, 2022

@nucleogenesis @rtibbles

Regarding this requirement

Renders, otherwise, only a RouterView, into which any implementing page in the app can route their behavior through.

I'm afraid that placing <router-view> into PageRoot itself is the same pattern we're trying to move away from and would soon cause us to be adding dependencies on pages into PageRoot, eventually ending with exactly the same issues we currently have with complicated conditions checking what page we're on to render this or that.

That's why I'd say it's important to have the main <router-view> in slim plugin index pages as @richard mentioned earlier in the refactor Notion doc, not in PageRoot itself, to allow us to use PageRoot from any page that will newly have its own page component which can use PageRoot as a template with its default behavior, customize it through props, slots, etc., or even not use it at all if the need arises. E.g. the template of the Bookmarks page would look like:

<template>
  <PageRoot>
    content specific to Bookmarks page
  </PageRoot>
</template>

I hope that from this example, it's possible to see that's it's not even possible to place <router-view> in PageRoot if we want to proceed with the architecture we discussed since every page would then render the main <router-view> which doesn't make much sense.

This means that PageRoot would contain the snackbar, errors, and notifications logic as described, but then would provide only <slot> instead of <router-view>, allowing for flexible use from anywhere we need thanks to moving most of the responsibility to pages that use PageRoot (and, if we need to have sub router views in some of our pages, we can always pass it through the slot).

@MisRob
Copy link
Member

MisRob commented Feb 14, 2022

Not sure yet if this would help or complicate things but alternatively, if the PageRoot is not supposed to contain any layout information and its purpose will be only and only things that we're sure we need on every page (errors, snackbars, ..) maybe it could be also used from plugin index pages so that there's no need to re-render its content from each page when navigating within one plugin.

In such a case, it'd be rather PluginRoot than PageRoot to be used only from plugin index pages, and PageRoot would be used in the same way from each page as described in the previous comment. But even in that case, it might still make sense to let plugin index pages define the main <router-view>? At least I don't see a reason why not to do so right now, but don't have much insight into the specifics of snackbars and error handling.

Example

Learn index page:

<template>
  <PluginRoot> // contains snackbars, error, and notifications handling
    <router-view>
  </PluginRoot>
</template>

Bookmarks page:

<template>
  <PageRoot> // contains default page layout and components (e.g. nav bars) that can be overridden via slots
    content specific to Bookmarks page
  </PageRoot>
</template>

@nucleogenesis
Copy link
Member Author

Noted. I misunderstood and that all makes sense. Will make a new issue.

@MisRob
Copy link
Member

MisRob commented Feb 15, 2022

@nucleogenesis Although the reasoning behind touches on more aspects of the high-level architecture, in terms of this particular issue it was basically all related to the last requirement only, so if that makes sense, I think it'd be also fine to just remove

Renders, otherwise, only a RouterView, into which any implementing page in the app can route their behavior through.

and maybe mention a bit about how we will use this component from other components instead. Whatever will work for you, just wanted to note that I don't think that the whole issue doesn't make sense.

@nucleogenesis
Copy link
Member Author

Thanks Misha - I closed the issue mostly because I didn't want to have it assigned to someone during the sprint planning before I fully grokked your comments.

I will update the router-view requirement to instead suggest a default slot.

In follow-up issues, then, such as "remove CoreBase from LearnIndex" the requirements will have the router-view put into that slot.

@nucleogenesis nucleogenesis reopened this Feb 16, 2022
@nucleogenesis nucleogenesis changed the title PageRoot is the new CoreBase New CoreBase Feb 17, 2022
@sairina sairina linked a pull request Feb 23, 2022 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 - critical Priority: Release blocker or regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants