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

Josh/step4 #12

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Josh/step4 #12

wants to merge 11 commits into from

Conversation

JoshLarouche
Copy link

Completed step 4 of the onboarding todo app

Copy link

@BCerki BCerki left a comment

Choose a reason for hiding this comment

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

Nice work! The comments that just say "b" are ones I need to go look something up to finish; I'll add more detail tomorrow

return (
<div className="App">
<header className="App-header">
<p>{data.allTasks.edges[0].node}</p>
Copy link

@BCerki BCerki Apr 20, 2023

Choose a reason for hiding this comment

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

I believe by this step, App should render your TodoList component because that's what contains all the todos at this point (also, if you want to make it clearer which work is part of which step, you can set the base branch for this branch to the one you used for step 3)

Comment on lines +6 to +8
import Card from "@mui/material/Card";
import CardContent from "@mui/material/CardContent";
import { Grid } from "@mui/material";
Copy link

Choose a reason for hiding this comment

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

FYI, when possible, we use components from this library: https://github.com/button-inc/service-development-toolkit. There's a bcgov theme that has colours, fonts, etc. built in, and progressive enhancement

import { Grid } from "@mui/material";

interface Props {
query: any;
Copy link

Choose a reason for hiding this comment

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

This should be the actual query type

);

console.log(data.allTasks);
const listItems = data.allTasks.edges.map((todo: { node: { id: any; }; }) =>
Copy link

Choose a reason for hiding this comment

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

We try to avoid the any type (but nice work including typing)

"storageKey": null
}
],
"type": "Task",
Copy link

@BCerki BCerki Apr 20, 2023

Choose a reason for hiding this comment

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

This is where the Task type comes from, and it's coming from the column named "task" in the table

-- requires: todo_appschema
-- requires: tasks

BEGIN;
Copy link

Choose a reason for hiding this comment

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

We use lowercase for sql commands

BEGIN;

-- XXX Add verifications here.
SELECT id, task, completed, date_created, date_updated
Copy link

Choose a reason for hiding this comment

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

If I remember correctly, verify only fails if there's an error. This won't error if the seed data went in wrong; it'll just return nothing. In this case, you could check that the count is greater than 0 or something (however, this one's not super relevant to our projects; we verify functions, tables, etc. but not seed data, and we usually check for existence)

BEGIN;

-- XXX Add verifications here.
SELECT id, task, completed, date_created, date_updated
Copy link

Choose a reason for hiding this comment

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

same note, won't fail

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.

2 participants