Tabris.js router by ahmadov · Pull Request #34 · eclipsesource/tabris-decorators

ahmadov · 2020-03-06T14:07:31Z

No description provided.

tbuschto · 2020-03-06T16:11:15Z

src/api/router/Route.ts

+
+export abstract class Route {
+  page: RoutePage;
+  options: RouteOptions = {


do we need this nesting? Why not directly on Route?

tbuschto · 2020-03-06T16:14:30Z

src/api/router/Router.ts

+
+  private _navigationView: NavigationView;
+  private _routes: ListLike<RouterConfig>;
+


No empty line

tbuschto · 2020-03-06T16:16:13Z

src/api/router/RouterHistory.ts

+
+export type HistoryItem = { route: string, payload?: any };
+
+export class RouterHistory<T extends HistoryItem = HistoryItem> extends ListLikeObvserver<T> {


An observer must never modify the obversvee. These methods should be router internals, or RouterHistory class it wraps, not extend the observer.

tbuschto · 2020-03-06T16:23:38Z

src/api/router/RouterHistory.ts

+export class RouterHistory<T extends HistoryItem = HistoryItem> extends ListLikeObvserver<T> {
+
+  public push(item: T) {
+    const source = Array.from(this.source);


This would replace the source even if it's a list. I'd rather deal with this using instanceof

tbuschto · 2020-03-06T16:28:55Z

src/api/router/Router.ts

+  }
+
+  set history(value: ListLike<HistoryItem>) {
+    if (this._routerHistoryObserver.source === value) {


Unnecessary optimization.

tbuschto · 2020-03-06T16:38:21Z

src/api/router/Router.ts

+    return this._routerHistoryObserver.source;
+  }
+
+  set routes(value: ListLike<RouterConfig>) {


Routes should be resolved via DI on demand (when they are needed). No property.

@Injectable('myRoute')
class MyRoute extends Route {

We COULD enable another way to give routes IN ADDITION, but I dont think we should allow removing/replacing routes after the router was created.

Can we resolve/create the object via class name? I'm trying to find the alternative ways to use the DI.

tbuschto · 2020-03-06T16:41:22Z

src/api/router/Router.ts

+  }
+
+  protected _handleHistoryChange = ({deleteCount, items}: Mutation<ItemType>) => {
+    if (deleteCount > items.length) {


But you don't know where in the history the items were deleted. This assumes it's at the end. It also doesn't handle replacement.

tbuschto · 2020-03-06T16:46:31Z

src/api/router/Router.ts

+  private _routerHistoryObserver: RouterHistory;
+  private _routerMatcher: RouterMatcher;
+
+  constructor({navigationView, routers, defaultRoute, history} : RouterProperties) {


hmmm.... do we need a default route? What's the use-case?

I think it's useful to navigate to the initial route

tbuschto · 2020-03-06T16:52:35Z

src/api/router/Router.ts

+
+  private _appendRoute(route: Route, payload?: any) {
+    if (route.page.onPayload && typeof route.page.onPayload === 'function') {
+      route.page.onPayload(payload);


The "payload" should be given to the route, not the page. Also, the entire item should be given so the router knows which kind of payload to expect. Also, it should just be a property like "item" or "historyItem", not a method with a name that sounds like listener.

I thought "payload" would be uninteresting to a route because, only the page reacts payload and will do UI changes.

the current implementation works as follows:

create all routes and pages (UI).

each page does UI changes based on the payload (that's why I named 'onPayload'). Even, the same page can use different payloads.

I've implemented without much investigation, maybe there are more robust alternatives to achieve the same behaviour.

tbuschto · 2020-03-06T16:53:30Z

src/api/router/Router.ts

+      route.page.onPayload(payload);
+    }
+    this._navigationView.append(route.page);
+    this._navigationView.drawerActionVisible = route.options.enableDrawer;


I think these two featues are a bit advanced for the first commit.

ahmadov force-pushed the tabris-router branch from c967a69 to 48ab10e Compare March 6, 2020 14:17

tbuschto requested changes Mar 6, 2020

View reviewed changes

tbuschto force-pushed the master branch 2 times, most recently from 36f18dc to e90fd70 Compare March 18, 2020 18:42

ahmadov force-pushed the tabris-router branch 2 times, most recently from 3e3ebfe to 3bd65a5 Compare April 9, 2020 12:30

[WIP] Router API

97621dd

ahmadov force-pushed the tabris-router branch 2 times, most recently from 44aac30 to 57e82ea Compare April 9, 2020 12:38

Router: add example

8664c1d

ahmadov force-pushed the tabris-router branch from 57e82ea to 8664c1d Compare April 9, 2020 12:43


		private _navigationView: NavigationView;
		private _routes: ListLike<RouterConfig>;


		export type HistoryItem = { route: string, payload?: any };

		export class RouterHistory<T extends HistoryItem = HistoryItem> extends ListLikeObvserver<T> {

Conversation

ahmadov commented Mar 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants