- Remove the login landing page #1345
Conversation
- Remove the back button as it is not needed anymore - Adjust layout due to the removal of backbutton and remove the propTypes requirement for onBack function
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…h-is-an-unnecessary-step
- Set step to 0 index
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Client/src/Pages/Auth/Login.jsx (2)
32-32: Yo dawg, the JSDoc above needs updating!The function documentation still mentions the
onBackprop that's been removed. Let's keep our docs fresh and clean!- * @param {Function} props.onBack - Callback function to handle "Back" button click.
Line range hint
314-325: Mom's spaghetti moment: Let's make these step values more maintainable!Consider defining step constants to make the code more maintainable and prevent magic numbers:
+ const LOGIN_STEPS = { + EMAIL: 0, + PASSWORD: 1 + }; - if (step === 0) { + if (step === LOGIN_STEPS.EMAIL) { // ... validation logic ... - setStep(1); + setStep(LOGIN_STEPS.PASSWORD); - } else if (step === 1) { + } else if (step === LOGIN_STEPS.PASSWORD) {
Client/src/Pages/Auth/Login.jsx
Outdated
| step === 1 && ( | ||
| <StepTwo | ||
| form={form} | ||
| errors={errors} | ||
| onSubmit={handleSubmit} | ||
| onChange={handleChange} | ||
| onBack={() => setStep(1)} | ||
| onBack={() => setStep(0)} |
There was a problem hiding this comment.
💡 Codebase verification
Yo, the back button's still alive and kickin' in StepTwo!
Looking at the code with spaghetti-stained glasses, we can see that StepTwo is actively using the back button:
- There's a fully implemented back button with
ArrowBackRoundedIcon - The
onBackprop is being used in the onClick handler - The component includes proper PropTypes validation for
onBack
This means the PR description about removing the back button doesn't match what's in the code - either the description needs updating or the back button still needs to be removed.
• Client/src/Pages/Auth/Login.jsx:180-197: Back button implementation is still present and functional
🔗 Analysis chain
Knees weak, arms are heavy: We've got a PropTypes inconsistency!
The PR objective was to remove the back button, but:
- StepTwo still has
onBackin its PropTypes definition - We're still passing
onBackto StepTwo
If we're keeping the back functionality in StepTwo, we should update the PR description. If not, we should remove these remnants.
Let's check if there are any other back button references:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining back button related code
rg -i "back.*button|onback|props\.onback" --type js
Length of output: 2860
Script:
#!/bin/bash
# Let's check the actual implementation of StepTwo in Login.jsx to see if it's using the back button
ast-grep --pattern $'const StepTwo = ({ $$$, onBack }) => {
$$$
}' Client/src/Pages/Auth/Login.jsx
# Also check if there are any visual back button elements in StepTwo
rg -A 5 -B 5 "back.*button|onback" Client/src/Pages/Auth/Login.jsx
Length of output: 8768
There was a problem hiding this comment.
Great, @shemy-gan ! On this one it would be a good opportunity to refactor and apply our rule of not having more than one component per file. Step One and Two could be their own component in their own file. The folder would be Login/components/StepOne and .../StepTwo
Client/src/Pages/Auth/Login.jsx
Outdated
| <Typography component="h1">Log In</Typography> | ||
| <Typography>We are pleased to see you again!</Typography> | ||
| </Box> | ||
| <Box width="100%"> | ||
| <Button | ||
| variant="outlined" | ||
| color="info" | ||
| onClick={onContinue} | ||
| sx={{ | ||
| width: "100%", | ||
| "& svg": { | ||
| mr: theme.spacing(4), | ||
| "& path": { | ||
| stroke: theme.palette.other.icon, | ||
| }, | ||
| }, | ||
| "&:focus-visible": { |
There was a problem hiding this comment.
Maybe we shouldn't ditch all of the initial greeting. We should check that with @gorkem-bwl . I'd keep
- We are pleased to see you again!
- By continuing, you agree to our Terms of Service and Privacy Policy.
Client/src/Pages/Auth/Login.jsx
Outdated
| @@ -433,9 +320,9 @@ const Login = () => { | |||
| setErrors((prev) => ({ ...prev, email: error.details[0].message })); | |||
| createToast({ body: error.details[0].message }); | |||
| } else { | |||
| setStep(2); | |||
There was a problem hiding this comment.
@ajhollid I know you requested this change, but this gets a bit confusing to me: we are on the component StepOne, but it is the step 0. If we are changing to step 0 I would suggest changing the name of the component as well
There was a problem hiding this comment.
@ajhollid I know you requested this change, but this gets a bit confusing to me: we are on the component StepOne, but it is the step 0. If we are changing to step 0 I would suggest changing the name of the component as well
Agreed, it is confusing. I agree changing the name of the component is a good idea anyways, StepOne is really not descriptive as to what Step One actually is.
@jennifer-gan can you please change the names when refactoring out the components? Thank you!
There was a problem hiding this comment.
sure, fixed here: c723342
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Client/src/Pages/Auth/Login/Components/EmailStep.jsx (1)
57-57: Simplify the boolean expression forerrorpropThe expression
errors.email ? true : falsecan be simplified for clarity.Apply this diff to refine the code:
-error={errors.email ? true : false} +error={Boolean(errors.email)}This keeps the code clean and straightforward.
🧰 Tools
🪛 Biome (1.9.4)
[error] 57-57: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
Client/src/Pages/Auth/Login/Components/PasswordStep.jsx (1)
74-74: Simplify the boolean expression forerrorpropSimilar to the previous comment, you can streamline the expression here.
Apply this diff to enhance readability:
-error={errors.password ? true : false} +error={Boolean(errors.password)}This makes the intent clear and the code neater.
🧰 Tools
🪛 Biome (1.9.4)
[error] 74-75: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Client/src/App.jsx(1 hunks)Client/src/Pages/Auth/Login.jsx(0 hunks)Client/src/Pages/Auth/Login/Components/EmailStep.jsx(1 hunks)Client/src/Pages/Auth/Login/Components/PasswordStep.jsx(1 hunks)Client/src/Pages/Auth/Login/Login.jsx(1 hunks)
💤 Files with no reviewable changes (1)
- Client/src/Pages/Auth/Login.jsx
🧰 Additional context used
🪛 Biome (1.9.4)
Client/src/Pages/Auth/Login/Components/EmailStep.jsx
[error] 55-55: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 57-57: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
Client/src/Pages/Auth/Login/Components/PasswordStep.jsx
[error] 74-75: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (2)
Client/src/Pages/Auth/Login/Login.jsx (1)
23-211: Great job setting up the new Login component!
The code is well-structured and reads smoothly. The flow between the email and password steps is seamless.
Client/src/App.jsx (1)
8-8: Update import path reflects the new structure
The change in the import path ensures the Login component is correctly referenced after restructuring.
| placeholder="[email protected]" | ||
| autoComplete="email" | ||
| value={form.email} | ||
| onInput={(e) => (e.target.value = e.target.value.toLowerCase())} |
There was a problem hiding this comment.
Avoid assigning directly within expressions
Assigning a value inside an expression can lead to unexpected behaviour. Instead of modifying e.target.value within the onInput, consider handling the lowercase transformation differently.
Apply this diff to fix the issue:
-onInput={(e) => (e.target.value = e.target.value.toLowerCase())}
+onChange={(e) => {
+ e.target.value = e.target.value.toLowerCase();
+ onChange(e);
+}}This ensures the value is transformed and the change is properly propagated.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 55-55: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
…h-is-an-unnecessary-step
ajhollid
deleted the
1321-remove-continue-with-email-page-which-is-an-unnecessary-step
branch
requirement for onBack function
as seen below
Screencast.from.2024-12-10.11-19-04.webm