Skip to content

30 frontend portal#39

Open
ashoomky wants to merge 26 commits intomainfrom
30-frontend-portal
Open

30 frontend portal#39
ashoomky wants to merge 26 commits intomainfrom
30-frontend-portal

Conversation

@ashoomky
Copy link
Copy Markdown
Collaborator

Description

implemented the styling for portal page

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Manual testing (requires screenshots or videos)
  • Integration tests written (requires checks to pass)

Checklist before requesting a review

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added thorough tests that prove my fix is effective and that my feature works
  • I have written a storybook for frontend components (if applicable)
  • I've requested a review from another user

@ashoomky ashoomky linked an issue Oct 19, 2025 that may be closed by this pull request
6 tasks
@ashoomky ashoomky requested a review from zlrkw11 October 19, 2025 08:29
Copy link
Copy Markdown
Member

@choden-dev choden-dev left a comment

Choose a reason for hiding this comment

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

good work!

Comment on lines +19 to +22
<form
className="flex flex-col gap-4"
onSubmit={(e) => e.preventDefault()}
>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: do we need to submit data here?

Comment on lines +8 to +10
const [fullName, setFullName] = useState("")
const [email, setEmail] = useState("")
const [password, setPassword] = useState("")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: consider what happens if the user leaves the page for a bit, will they be able to keep their data?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are good libraries for handling this like https://react-hook-form.com/

value={password}
onChange={(e) => setPassword(e.target.value)}
placeholder="Example Password"
autoComplete="current-password"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: do we want users to sign up with a current password or a new password - this is a sign up page

Comment on lines +37 to +43
<style>{styles}</style>
<div
className={`grad-border p-15 max-w-md w-full shadow-lg ${className}`}
style={{
backgroundColor: "rgba(185, 213, 255, 0.25)",
backdropFilter: "blur(12px)",
}}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: it is an anti-pattern to mix tailwind styling and inline css/style tags, consider defining the styles you need in the globals.css file OR look up the relevant tailwind alternatives

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

concatenating classname like:

`grad-border p-15 max-w-md w-full shadow-lg ${className}`

can lead to unexpected errors, consider using the cn() function in client/utils like:

cn("grad-border p-15 max-w-md w-full shadow-lg", className)

which will handle any edge cases

Copy link
Copy Markdown
Member

@jji05 jji05 Oct 19, 2025

Choose a reason for hiding this comment

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

^ requires your branch to be up to date with main branch

Copy link
Copy Markdown
Member

@jji05 jji05 left a comment

Choose a reason for hiding this comment

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

Looks good generally, also I'd recommend taking a quick read in conventional commits.

https://www.conventionalcommits.org/en/v1.0.0/

I noticed that a couple commit messages are recommended to be labeled as a different type such as:

  • feat: ran prettier (should be style: ...)
  • ran prettier -> run prettier (no past tense in description)

Copy link
Copy Markdown
Member

@jji05 jji05 Oct 19, 2025

Choose a reason for hiding this comment

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

question: a signup page already exists, are you sure this page is still needed?

existing signup page:

Image

sign-up page in this PR:

Image

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FRONTEND] Portal

7 participants