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
Open

Fixed: The add button is not disabled #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb5057a
Synced local files to container files
PadamSinha2511 Aug 15, 2024
1231584
Merge branch 'main' into main
PadamSinha2511 Aug 16, 2024
d91be98
Save button only enables when repo url is valid
PadamSinha2511 Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions compose.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

env_file:
- .env
environment: # To override the DB URL in the .env file
Expand Down
26 changes: 22 additions & 4 deletions src/app/account/repo/add/form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { SubmitButton } from "@/components/forms/SubmitButton";
import Input from "@/components/forms/Input";
import Checkbox from "@/components/forms/Checkbox";
import Alert from "@/components/Alert";
import {useState,useEffect} from "react"

const initialState = {
data: undefined,
Expand All @@ -16,8 +17,22 @@ const initialState = {

export default function Form({ usage }) {
const [state, formAction] = useFormState(getRepo, initialState);
const disabled = usage >= process.env.NEXT_PUBLIC_REPO_LIMIT ? true : false;
const [usageLimitReached,setUsageLimitReached] = useState(false)
eddiejaoude marked this conversation as resolved.
Show resolved Hide resolved
const [githubUrl , setGithubUrl] = useState(state?.data?.url || '')
const [isGithubUrlValid,setIsGithubUrlValid] = useState(false)
const [isDisabled,setIsDisabled] = useState(true)

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 👍


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


setUsageLimitReached(usage >= process.env.NEXT_PUBLIC_REPO_LIMIT);
setIsGithubUrlValid(githubRegex.test(githubUrl))

setIsDisabled(usageLimitReached || !isGithubUrlValid)

},[usage,githubUrl])

return (
<form action={formAction}>
<Alert>
Expand All @@ -30,13 +45,15 @@ export default function Form({ usage }) {
</p>

<div className="mt-10 grid grid-cols-1 gap-x-6 gap-y-8 sm:grid-cols-6">

<Input
id="url"
name="url"
text="GitHub repo URL"
error={state?.errors?.url}
value={state?.data?.url}
disabled={disabled}
value={githubUrl}
onChange={(e)=>setGithubUrl(e.target.value)}
disabled={usageLimitReached}
/>
</div>
</div>
Expand Down Expand Up @@ -79,7 +96,8 @@ export default function Form({ usage }) {
</fieldset>

<div className="mt-6 flex items-center justify-end gap-x-6">
<SubmitButton text="SAVE" disabled={disabled} />
<SubmitButton text="SAVE" disabled={isDisabled} />

</div>
</form>
);
Expand Down
2 changes: 2 additions & 0 deletions src/components/forms/Input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export default function Input({
error,
prepend,
disabled = false,
onChange,
type = "text",
}) {
const { pending } = useFormStatus();
Expand All @@ -28,6 +29,7 @@ export default function Input({
type={type}
disabled={pending || disabled ? true : false}
name={id}
onChange={onChange}
id={id}
className={classNames(
"block w-full rounded-md border-0 sm:text-sm sm:leading-6 bg-gray-800 disabled:bg-gray-600",
Expand Down
Loading