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

Matt Painter - Code Challenge Solution. #5

Open
wants to merge 5 commits into
base: challenge-response/matt-painter
Choose a base branch
from

Conversation

Matt-Painter
Copy link

does not pull data in from api dynamically. Is using static results.
has non-functioning "submit" button on filter.
no clear button for filter, but does clear out when emptying the filter fields.

return {
getDashboardData: async () => {

return [
Copy link
Contributor

Choose a reason for hiding this comment

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

import { Grid } from '@material-ui/core';

function Card(props){
const item = props.item;
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't really need to alias item here. A better pattern would be to destructure props in the function signature, e.g.

function Card({item}) {
...

const item = props.item;
return (
<Grid container onClick={() => props.buttonAction(item.id)}>
<Grid item xs={10}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the default in your IDE is to autocomplete attributes to {}, but when you're using a static value instead of a calculated one, you can use quotes, e.g.

<Grid item xs="10">

</ul>
</Grid>
<Grid item xs={2}>
<Button variant="contained" color="primary">{props.buttonText}</Button>
Copy link
Contributor

Choose a reason for hiding this comment

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

If you end up destructuring your props, make sure to add buttonText as well:

function Card({item, buttonText}) {
...

Then you can just use {buttonText} here.

<h3>Filter Input</h3>
<form>
<label></label>
<input type="text" onChange={(event) => props.onFilterChange(event.target.value)}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

onFilterChange prop could be destructured above.


function List(props){

console.log("here");
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to clean up console.log()s when you submit a PR! I usually end up opening one, reading through it once, and then committing a "cleanup" push.


function onFilterChange(text){
console.log(text);
console.log("filter");
Copy link
Contributor

Choose a reason for hiding this comment

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

Stray console.log()s

@@ -1,6 +1,6 @@
import * as React from "react";
import { Route, withRouter } from "react-router";
import { DashboardPage } from "./dashboard";
import { DashboardPage } from "./dashboard/index";
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this was autocorrected by your IDE, but one of the quirks of import is that if you have a folder, and an index.js inside of it, you don't strictly need to reference <folder>/index.

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