-
Notifications
You must be signed in to change notification settings - Fork 3
feat: user can set a custom url code #3
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
base: master
Are you sure you want to change the base?
Conversation
I added a new feature that allows user set a custom url code, and simplified .env variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I think the variable names are enough to explain their role. Adding extra files will not be of any use.
Implement Form Validation at Backend Register
@joaovitorzv, as I have mentioned in the comments, the custom URL code, since do not have a backend integration is of no use. And secondly, the variables are, I guess self-explanatory by their names, so I think there is no need for the extra file. So you can make PR which includes changes other than that which I will merge after reviewing. I hope this will help. 🙂 |
routes/api/url.js
Outdated
const baseUrl = process.env.BASE_URL; | ||
if (!validUrl.isUri(baseUrl)) { | ||
return res.status(400).send({ msg: "Invalid Base Url" }); | ||
} | ||
|
||
const urlCode = shortId.generate(); | ||
let urlCode; | ||
if (customCode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @joaovitorzv
Add this custom code checking after valid URL check and checking if the URL already exists. Since there is no meaning of checking if the Long URL is not valid or the URL already exists in the DB.
Hey just make the new PR with the mentioned changes. The feature is fine. I was also thinking about adding a custom code feature but skipped to let the App be simple. But yes this feature is cool, therefore adding it now. 🙂 |
added custom code checking after url check and if longUrl exists
That way future contributors can easily know what env vars exists on project