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

Fix incomplete submit #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

marcellinuselbert
Copy link
Member

@marcellinuselbert marcellinuselbert commented Oct 19, 2021

Revision of #10
bug:
Users can submit without a long URL is filled.

request feature:
Add toast to handle disabled button is clicked

old behavior :

Submit button isn't disabled and users can submit without a long URL, when users trying to click the disabled button there's no toast pop up

image

new behavior :

  • Submit button is disabled, also users can't submit without long URL is filled.

image

  • There's toast when users trying to click the disabled button based on missing values such as, Please enter long URL and short URL, etc.

image

You can try to submit without long URL and short URL, short URL only, long URL only on RISTEK-LINK-PREVIEW

Thanks 😀

@jonathanfilbert
Copy link
Member

Hey Marcel, thank you for submitting the new PR. Can I have the new deploy preview for this PR?

Copy link
Member

@jonathanfilbert jonathanfilbert left a comment

Choose a reason for hiding this comment

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

Great work! However, there are a few things that could be improved. I've reviewed the code, please submit your revisions if you're done.

Comment on lines 83 to 99
else if (url === "" && alias === "") {
toast({
title: "Error occured",
description: "Please enter long url and short url",
status: "error",
duration: 5000,
isClosable: true,
});
} else if (alias === "") {
toast({
title: "Error occured",
description: "Please enter short url",
status: "error",
duration: 5000,
isClosable: true,
});
} else if (url === "") {
Copy link
Member

Choose a reason for hiding this comment

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

can we refactor this? the if statements are quite confusing and it's not readable. One thing to consider is to use a variable for each condition for example:

if (alias === ''){}

to

const isAliasEmpty = alias === ''

and then you can refactor the ifs into a more elegant way like:

if (isAliasEmpty){
toast({title: "Alias Empty"})
}


useEffect(() => {
if (!!alias && isUrlValid) {
if (!!alias && isUrlValid && url !== "") {
Copy link
Member

Choose a reason for hiding this comment

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

Refactor these boolean conditions into a more readable variable e.g. urlValid

@@ -15,6 +15,7 @@
"axios": "^0.21.1",
"dotenv": "^9.0.0",
"framer-motion": "^4",
"gzip-cli": "^1.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

why would we need gzip in our dependency? If this is unused, please remove.

@marcellinuselbert
Copy link
Member Author

Hello! I've redeployed this preview on Ristek Preview. I've submitted the revision too, please kindly check my commit. @jonathanfilbert

Comment on lines 86 to 92
toast({
title: "Error occured",
title: "Long url and short url empty",
description: "Please enter long url and short url",
status: "error",
duration: 5000,
isClosable: true,
});
Copy link
Member

Choose a reason for hiding this comment

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

This toast function is a bit redundant. It's the same toast function but used multiple of times in this file. Is there any way we can make this simpler? Maybe wrap the toast function in a reusable component called Toast with props like:

ToastProps = {
title: String,
description: String,
isError?: Boolean, // this one defaults to false
}

the duration and isClosable don't need to be exported into props because they're default behavior.

So everytime we want to call the Toast, all we have to do is just:

<Toast isError title="Long URL empty" description="Please enter long url" />

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants