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

Convert to a Next + Redux app #35

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

Convert to a Next + Redux app #35

wants to merge 39 commits into from

Conversation

JacksonMeade
Copy link
Collaborator

No description provided.

@jakebromberg
Copy link
Member

Thanks for doing this, Jackson. Couple suggestions:

  1. Please add a description for the PR.
  2. This is a very large PR. Please break it down into more than two commits, if possible. Breaking it into multiple PRs might be even better.
  3. Could you attach this to a ticket? This will help us in the future if we need to come back to this with more PRs.

@JacksonMeade
Copy link
Collaborator Author

Noting here that I will be writing more extensive documentation and giving y'all a full rundown on Saturday; while this is in a draft state don't worry about reviewing.

@jakebromberg
Copy link
Member

jakebromberg commented Jan 30, 2024 via email

Why is the AWS sdk documentation so atrocious? Like, do you want us to use this or not?

const AdminPage = () => {
return (
Copy link
Member

Choose a reason for hiding this comment

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

Could you fix the indentation, please?

import { getter } from "@/lib/services";
import { useEffect } from "react";
import { toast } from "sonner";

export default function VerifyPage() {

const user = useSelector(getAuthenticatedUser);
const dispatch = useDispatch();

useEffect(() => {
console.log("mounted");

const process = async (e: KeyboardEvent) => {
Copy link
Member

Choose a reason for hiding this comment

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

Could you rename this to something more descriptive like processKeyboardInput or something?

import { getter } from "@/lib/services";
import { useEffect } from "react";
import { toast } from "sonner";

export default function VerifyPage() {

const user = useSelector(getAuthenticatedUser);
const dispatch = useDispatch();

useEffect(() => {
console.log("mounted");

const process = async (e: KeyboardEvent) => {
if ((e as KeyboardEvent).key === 'v') {
Copy link
Member

Choose a reason for hiding this comment

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

Could you leave a comment that documents why v is special? Is this v for verify?

lib/redux/model/authentication/thunks.ts Show resolved Hide resolved
lib/redux/model/flowsheet/slice.ts Show resolved Hide resolved
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