Skip to content

Conversation

trcazier
Copy link

No description provided.

@robodoo
Copy link

robodoo commented Sep 22, 2025

Pull request status dashboard

@trcazier trcazier requested a review from cgun-odoo September 22, 2025 14:19
Copy link

@cgun-odoo cgun-odoo left a comment

Choose a reason for hiding this comment

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

Amazing job

Copy link

@cgun-odoo cgun-odoo left a comment

Choose a reason for hiding this comment

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

You named your commit sections 8-9 but this contains section 10 🤨

@trcazier trcazier force-pushed the 18.0-web-tutorial-trcaz branch from 88c95bf to bc43839 Compare September 23, 2025 08:19
@trcazier trcazier force-pushed the 18.0-web-tutorial-trcaz branch from b73ff1c to 9d8ff02 Compare September 23, 2025 08:30
@trcazier trcazier force-pushed the 18.0-web-tutorial-trcaz branch from 8630778 to 143eec9 Compare September 23, 2025 08:55

export class PieChart extends Component {
static template = "awesome_dashboard.pie_chart";
static props = { data: {optional: false}}

Choose a reason for hiding this comment

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

Every prop is required by default. You don't have to write it unless it's optional: true

Copy link
Author

Choose a reason for hiding this comment

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

I know but I just didn't want to type check the data because it's json and annoying but I still wanted to require the data prop

}

setup() {
this.size = this.props.size || 1

Choose a reason for hiding this comment

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

You can also use defaultProps

async function loadData() {
console.log("slt")
const newStats = await rpc("/awesome_dashboard/statistics");
Object.keys(newStats).forEach((k) => stats[k] = newStats[k])

Choose a reason for hiding this comment

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

why can't you do something like stats = newStats?
why did you have to iterate over the keys?

Copy link
Author

Choose a reason for hiding this comment

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

When I did that it didn't register the change, maybe because it overwrote the useState, idk
I can try again

Copy link
Author

Choose a reason for hiding this comment

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

Nope doesn't work with stats = newStats

Choose a reason for hiding this comment

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

Object.assign(stats, newStats) maybe

Copy link
Author

Choose a reason for hiding this comment

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

How on god's green earth was I supposed to find that out

@trcazier trcazier force-pushed the 18.0-web-tutorial-trcaz branch from 14a06af to 82c0d40 Compare September 24, 2025 11:24
@cgun-odoo
Copy link

image What happened here

</t>

<t t-foreach="items" t-as="item" t-key="item.id">
<DashboardItem size="item.size">

Choose a reason for hiding this comment

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

What happens if they don't have size? I saw that you don't have a default value

Copy link
Author

Choose a reason for hiding this comment

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

this.size = this.props.size || 1

@trcazier
Copy link
Author

image What happened here

I amended the last commit but I had already pushed so it created a new branch I guess

import { _t } from "@web/core/l10n/translation";
import { DashboardItem } from "./dashboard_item";
import { PieChart } from "./pie_chart/pie_chart";
import { browser } from "@web/core/browser/browser";

Choose a reason for hiding this comment

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

👀

Copy link
Author

Choose a reason for hiding this comment

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

?

this.stats = useState(useService("awesome_dashboard.statistics"));
this.items = registry.category("awesome_dashboard").getAll();
this.dialog = useService("dialog");
this.state = useState({ itemsNotShown: browser.localStorage.getItem("itemsNotShown")?.split(",") || []});

Choose a reason for hiding this comment

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

Suggested change
this.state = useState({ itemsNotShown: browser.localStorage.getItem("itemsNotShown")?.split(",") || []});
this.state = useState({ itemsNotShown: localStorage.getItem("itemsNotShown")?.split(",") || []});

just localStorage should be enough. Or maybe I'm dreaming 📦

Copy link
Author

Choose a reason for hiding this comment

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

idk I had no idea how to do it so I just looked it up

this.stats = useState(useService("awesome_dashboard.statistics"));
this.items = registry.category("awesome_dashboard").getAll();
this.dialog = useService("dialog");
this.state = useState({ itemsNotShown: browser.localStorage.getItem("itemsNotShown")?.split(",") || []});

Choose a reason for hiding this comment

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

It's confusing to have a state variable called state when you already have other state variables (stats). Either put stats in state or take itemsNotShown outside of state

Copy link
Author

Choose a reason for hiding this comment

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

But isn't state used to pass mutable variables? I put the ones that change in this.state and the others just in this

Choose a reason for hiding this comment

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

I never heard a rule like that, did you see it somewhere 👀

Copy link
Author

Choose a reason for hiding this comment

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

No but it makes more sense to me :)

Comment on lines 53 to 74
class SettingsDialog extends Component {
static template = "awesome_dashboard.SettingsDialog";
static components = { Dialog, CheckBox };
static props = ["dashboard", "items", "itemsNotShown"];

setup() {
this.items = useState(this.props.items);
this.items.forEach((item) => { item.shown = !this.props.itemsNotShown.includes(item.id)});
}

toggleItem (ev, item) {
item.shown = ev;

const newItemsNotShown = Object.values(this.items)
.filter((i) => !i.shown)
.map((i) => i.id)

browser.localStorage.setItem("itemsNotShown", newItemsNotShown);

this.props.dashboard.state.itemsNotShown = newItemsNotShown;
}
}

Choose a reason for hiding this comment

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

Deserves its own file imo

Copy link
Author

Choose a reason for hiding this comment

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

The tutorial solution doesn't share the same opinion

Copy link

@cgun-odoo cgun-odoo Sep 25, 2025

Choose a reason for hiding this comment

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

🤠 are we looking at the solution now

Copy link
Author

@trcazier trcazier Sep 25, 2025

Choose a reason for hiding this comment

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

Well when the exercise says "code something you have no idea how to do and we give you no docs for with components you don't know the name of in a framework you don't know" I don't feel like trying


browser.localStorage.setItem("itemsNotShown", newItemsNotShown);

this.props.dashboard.state.itemsNotShown = newItemsNotShown;

Choose a reason for hiding this comment

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

uuuhh so you're getting the parent as a prop, then changing the state of the parent in the child. Why don't you handle all of this in the parent. You can create the toggleItem (or similar) function in the parent and pass the function to the child as a prop

Copy link
Author

@trcazier trcazier Sep 25, 2025

Choose a reason for hiding this comment

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

I was gonna write a witty answer but instead I'll just commit another solution that will comply entirely with what I was gonna write & will probably irritate you even more

Copy link
Author

Choose a reason for hiding this comment

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

I hope you like it

setup() {
this.items = useState(this.props.items);
this.items.forEach((item) => { item.shown = !this.props.itemsNotShown.includes(item.id)});
this.items.forEach((item) => { item.shown = !this.props.dashboard.state.itemsNotShown.includes(item.id)});

Choose a reason for hiding this comment

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

I hate everything about this.
Good job 👍

Copy link
Author

Choose a reason for hiding this comment

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

That way I only have to pass 1 prop: the dashboard, and not: itemsNotShown and 15update methods 😄

Comment on lines +14 to +16
if (this.pieChart) {
this.pieChart.destroy();
}

Choose a reason for hiding this comment

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

Suggested change
if (this.pieChart) {
this.pieChart.destroy();
}
this.pieChart?.destroy();

this.buildChart();
});

onWillUnmount(() => { if (this.pieChart) {this.pieChart.destroy()}; });

Choose a reason for hiding this comment

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

Suggested change
onWillUnmount(() => { if (this.pieChart) {this.pieChart.destroy()}; });
onWillUnmount(() => {this.pieChart?.destroy(); });

Copy link
Author

Choose a reason for hiding this comment

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

Hum hum...

#967 (comment)

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.

3 participants