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

Builds the usability survey page #315

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Builds the usability survey page #315

wants to merge 17 commits into from

Conversation

georgeweiler
Copy link
Contributor

Builds the usability survey form. There are a few TODOs to implement in future pull requests:

  1. Small screen styling
  2. Posting the results of the form
  3. Pagination

@github-actions
Copy link

@georgeweiler georgeweiler changed the title Builds the usability survey page WIP: Builds the usability survey page Nov 28, 2022
@georgeweiler georgeweiler changed the title WIP: Builds the usability survey page Builds the usability survey page Nov 28, 2022
@github-actions
Copy link

github-actions bot commented Dec 1, 2022

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

{range(numberOfButtons, 1).map((n) => (
<Button
size={size}
variant="sequence"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's bump the components package- the PR that adds new variant has been merged threshold-network/components#37.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@r-czajkowski doesn't the development version cover this case? Is it just a yarn.lock update we need? or do you suggest we install "@threshold-network/components": "^1.0.0-dev.0",?

Copy link
Collaborator

@r-czajkowski r-czajkowski left a comment

Choose a reason for hiding this comment

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

First round of review done

Comment on lines 73 to 82
rows.map((row) => {
if (row.id === rowId) {
return {
...row,
value,
}
}
return row
})
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we simplify?

setRows(rows => rows.map(row => row.id === rowId ? { ...row, value } : row))

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

Copy link
Collaborator

@r-czajkowski r-czajkowski left a comment

Choose a reason for hiding this comment

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

Looks like we have 0 additions but 25k deletions in yarn.lock. Are we sure everything is OK with yarn.lock?

import { FC } from "react"
import { Button, HStack } from "@threshold-network/components"
import { range } from "../utils/range"
import { StackProps } from "@chakra-ui/react"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we import from @threshold-network/components?

numberOfButtons: number
selectedButtonNum?: number
onButtonClick: (n: number) => void
size?: ThemeTypings["components"]["Button"]["sizes"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can ButtonProps["size"] and import the ButtonProps from @threshold-network/components

Tr,
} from "@threshold-network/components"
import { Row, RowID, RowValue } from "./types"
import { useColorModeValue } from "@chakra-ui/react"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's import from @threshold-network/components.

value,
}))

console.log("Payload ", payload)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need console.log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove it. just added to prevent an unused var warning. I still need to implement the post function, but waiting for code from other branches.

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