maorb - Web Framework Tutorial

[Mark-Orban]

No description provided.

[robodoo]

[Megaaaaaa]

Hello hello 👋

Here's a review for you! Looks pretty good, it's only a few comments about stuff that we tend to do another way and that I thought it'd be best to show you. 👍

Tell me if anything is unclear or if you have any question!

[Megaaaaaa]

Any slot meant for a component is usually declared inside the component. As this is not the first time I see this, I suppose this is how they tell you to do it in the tutorial but in my experience, we will always put it inside the component. Just so you know 👍

Suggested change

	<t t-set-slot="layout-buttons">
	<button class="btn btn-primary" t-on-click="openCustomers">Customers</button>
	<button class="btn btn-primary" t-on-click="openLeads">Leads</button>
	</t>
	<Layout className="'o_dashboard h-100'">
	<Layout className="'o_dashboard h-100'">
	<t t-set-slot="layout-buttons">

[Megaaaaaa]

The chart data comes from a statistics service that is filled asynchronously, so it is normal for statistics.orders_by_size to be missing on the first render. Declaring data as required makes the child component stricter than the parent data flow can guarantee. Let's make it optional for safety measures 👍

Suggested change

	data: Object,
	data: {
	type: Object,
	optional: true,
	},

[Megaaaaaa]

Don't overlook that onPatched can run before a chart exists, especially while the async statistics are still loading. Leaving this unguarded accepts a runtime error path during a normal loading state.

Suggested change

	this.chart.destroy();
	this.chart?.destroy();

[Megaaaaaa]

Because the dashboard statistics are loaded asynchronously, this.props.data can be undefined when the component first renders and calling Object.keys on undefined would crash. Just normalize the props and we're good to go 👍

Suggested change

	const labels = Object.keys(this.props.data);
	const data = Object.values(this.props.data);
	const chartData = this.props.data || {};
	const labels = Object.keys(chartData);
	const values = Object.values(chartData);

[Megaaaaaa]

unused import 😉

Suggested change

	import { Component, markup, useState } from "@odoo/owl";
	import { Component, useState } from "@odoo/owl";

[Megaaaaaa]

Yo ucan use Owl's .bind syntax for passing component methods to children. It works just as good but looks a bit better and also, it's the odo oway 😄

Suggested change

	<Counter onChange="() => this.incrementSum()"/>
	<Counter onChange.bind="incrementSum"/>

[Megaaaaaa]

I suppose this is the way the tutorial tells you to do it as I've seen it multiple times but for your information, when we make an action clickable, we really want to put it in a because it works just as good with the mouse but helps a lot with the keyboard usage and also some assistive controls. Don't make much a difference here but now you know 👍

Suggested change

	<span class="fa fa-remove" t-on-click="onRemove"/>
	<button type="button" class="btn btn-link p-0" t-on-click="onRemove">
	<i class="fa fa-remove"/>
	</button>

[Megaaaaaa]

We prefer using the key name instead of the key code as it is much more self explanatory and we like self explanatory. "When a user clicks 'Enter' then this happens." :)

Also I would have trimmed the input to make sure we do no accept only spaces as an input but that's a preference thing. Altough if you decide to do that, then you need to save event.target.value.trim() instead of event.target.value right below. 👍

Suggested change

	if (event.keyCode === 13 && event.target.value != "") {
	if (event.key === "Enter" && event.target.value.trim() !== "") {