Skip to content

status icons for mailing list form including loading icon #125

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

adamgunn
Copy link
Collaborator

@adamgunn adamgunn commented May 6, 2023

No description provided.

@KindaOK KindaOK self-requested a review May 7, 2023 02:43
Copy link
Collaborator

@KindaOK KindaOK left a comment

Choose a reason for hiding this comment

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

Looks good. Just needs some minor code quality changes. Also would like some sort of Toast message. Can be something really simple that pops up over/under the input for a few seconds.

export default function MailingList() {
const [address, setAddress] = useState("");
const [status, setStatus] = useState(READY_STATUS);
const [disabled, setDisabled] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Disabled state is redundant since it's only ever true when status === LOADING_STATUS

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can just be like const disabled = status === LOADING_STATUS

100% {
transform: rotate(360deg);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Linter says semicolon is missing here. Can't see the github thing for suggesting a specific change.

) : status === SUCCESS_STATUS ? (
<FontAwesomeIcon icon={["fas", "check"]} />
) : (
<FontAwesomeIcon icon={["fas", "times"]} />
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 a restart icon would be more effective in telling the user to try resubmitting.

})
});
const statusToTitle = {};
statusToTitle[READY_STATUS] = "Send";
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 this part would be better if it was an object like {title: "Send", icon: "paper-plane"} to avoid that nasty nested ternary. Alternatively, move the setting of the icon to a variable and use switch instead.

e.preventDefault();
setResult(null);
const READY_STATUS = "ready";
const LOADING_STATUS = "loading";
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's more idiomatic to use an object mapping to strings for enums.

const Status = {
  READY: "READY"
  ...
}

);
return (
<EmailForm onSubmit={addEmailToList}>
<EmailInputBox
Copy link
Collaborator

Choose a reason for hiding this comment

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

It could use a Toast message to indicate completeness or to spit out the error message from the server.

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