Skip to content
This repository has been archived by the owner on Apr 10, 2023. It is now read-only.

Added visual cueing when submitting forms #52

Merged
merged 3 commits into from
Jan 29, 2023
Merged

Conversation

darinmclain
Copy link
Contributor

Updated submit buttons with loaders upon submission, added success alerts

Changed ErrorProvider to become NotificationProvider, added success functionality and properties to that context.

@darinmclain darinmclain requested a review from a team as a code owner January 28, 2023 04:49
Copy link
Contributor

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few minor things

@@ -1,7 +1,22 @@
export default function Loading() {
type LoadingProps = {
isButton?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think this is actually used anywhere else, the Loading component, so we could probably just make this only apply to buttons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I wasn't sure if this was planned to be use elsewhere. Removed the isButton logic/prop

@@ -44,6 +48,9 @@ export default function VariantForm(props: VariantFormProps) {
handleSubmit(values)
.then(() => {
clearError();
setSuccess(
`Successfully ${submitPhrase.toLocaleLowerCase()}d variant.`
Copy link
Contributor

Choose a reason for hiding this comment

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

in some cases we end this message with a . and others we don't, like: https://github.com/flipt-io/flipt-ui/pull/52/files#diff-6fdc396f20dbeed901b28539b86f9ee4c7097a9f8aae89882a0c374dddfbb099R51

We should probably be consistent in the messaging/punctation, I'm just not sure which one I prefer (with the period or without). Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I actually like without the .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the . from success messages

const { success, setSuccess } = useSuccess();

return (
<Transition show={success !== null}>
Copy link
Contributor

Choose a reason for hiding this comment

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

could we make the success notification clear itself after a short period? say 2-3 seconds? i think success notifications should be short-lived, while error notifications should persist until manually closed/acknowledged. wyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good thought, I wasn't sure if this was a desired functionality. Set timeout to be 3 seconds for now.

…od, set timeout to 3 seconds for success messages, made the loading component just be for buttons
Copy link
Contributor

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

lgtm! thank you!

@markphelps markphelps merged commit 7de935d into main Jan 29, 2023
@markphelps markphelps deleted the 153-visual-cue branch January 29, 2023 15:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants