Skip to content
This repository has been archived by the owner on Oct 18, 2024. It is now read-only.

Fixed: The add button is not disabled #176

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

Conversation

PadamSinha2511
Copy link
Contributor

Closes #149

Changes proposed

Now the save button is disabled for empty as well as for invalid repo URLs, it only enables If the repo URL is valid.

Screenshots

Screencast.from.2024-08-16.16-50-05.webm

Copy link
Member

@Bashamega Bashamega left a comment

Choose a reason for hiding this comment

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

Please fix the failing tests

@PadamSinha2511
Copy link
Contributor Author

Please fix the failing tests

Sure will look into it

@@ -3,6 +3,9 @@ services:
build:
context: .
dockerfile: Dockerfile.dev
volumes:
- .:/usr/src/app
- /usr/src/app/node_modules
Copy link
Member

Choose a reason for hiding this comment

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

please do not use main in your fork, because these changes should not be in your 2nd PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So i should checkout to some other branch and then push the changes.Right?

Copy link
Member

Choose a reason for hiding this comment

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

You should always create a new branch from main, but you never push to main, your changes only enter main when your PR is accepted and merged, then you update your main to be in sync with upstream

I have a video demoing and explain on my youtube channel


const githubRegex = /^https:\/\/github\.com\/[a-zA-Z0-9_-]+\/[a-zA-Z0-9_-]+$/;
Copy link
Member

Choose a reason for hiding this comment

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

My concern with this, is we should let github return a 404 if the repo is not found - if we try to do it with a regex we might be stopping people adding repos

Copy link
Member

Choose a reason for hiding this comment

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

After more thought, I am concerned that this might present other bugs in the future

Copy link
Member

Choose a reason for hiding this comment

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

My concern with this, is we should let github return a 404 if the repo is not found - if we try to do it with a regex we might be stopping people adding repos

I suggested regex so people can't enter non repo links. Like git lab links or spam links, or anything else

Copy link
Member

Choose a reason for hiding this comment

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

I think the UX is better, but could create a bug without us knowing. At the moment we rely on github returning a 200 or 404 which after some thought I think is better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay so I think we could only check for the domain using regex i.e. whatever the URL is being entered has to be a GitHub URL and let's not check further as if the repo does not exist the GitHub itself will return a 404.

Example:
https://google.com/EddieHubCommunity/HealthCheck --> is a invalid URL
https://github.com/EddieHubCommunity/Reop --> returns a 404 from github
https://github.com/EddieHubCommunity/HealthCheck --> a valid URL

Copy link
Member

Choose a reason for hiding this comment

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

great idea, that is a great idea and comprise 👍


const githubRegex = /^https:\/\/github\.com\/[a-zA-Z0-9_-]+\/[a-zA-Z0-9_-]+$/;

useEffect(()=>{
Copy link
Member

Choose a reason for hiding this comment

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

Please format files to our project standards

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

Successfully merging this pull request may close these issues.

The add button is not disabled
3 participants