Skip to content
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.

Sign up for GitHub

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

fix: integrate TextInput #1202

Merged
merged 25 commits into from
Nov 30, 2024
Merged

fix: integrate TextInput #1202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
12767d5
integrate TextField into create infrastructure page
ajhollid Nov 26, 2024
48c3f05
integrate TextInput
ajhollid Nov 26, 2024
a9493db
integrate TextInput into login page
ajhollid Nov 26, 2024
7b392bc
expose on TextInput
ajhollid Nov 26, 2024
475e927
integrate TextInput into register page
ajhollid Nov 26, 2024
9d8ea82
Merge pull request #1205 from bluewave-labs/fix/fe/text-input-register
ajhollid Nov 26, 2024
0a3d1c3
Merge pull request #1204 from bluewave-labs/fix/fe/login-text-input
ajhollid Nov 26, 2024
3bfdbf5
Merge pull request #1203 from bluewave-labs/fix/fe/create-monitor-tex…
ajhollid Nov 26, 2024
2bfefcb
Expose flex on TextInput
ajhollid Nov 26, 2024
9e3a2bb
integrate TextInput into profile panel
ajhollid Nov 26, 2024
817bbb1
Adjust helper text spcing, expose hidden prop
ajhollid Nov 26, 2024
efd0d68
integate TextInput into PasswordPanel
ajhollid Nov 26, 2024
f81b595
expose margin, integrate into TeamPanel
ajhollid Nov 26, 2024
74e9f19
integrate TextInput into advanced settings page
ajhollid Nov 26, 2024
bdfe91c
integrate into ForgotPassword page
ajhollid Nov 26, 2024
3ff9412
Integrate into SetNewPassword
ajhollid Nov 26, 2024
bbcf326
Remove unused imports, integrate into CreateMaintenance
ajhollid Nov 26, 2024
1334645
integrate into configure
ajhollid Nov 26, 2024
19eceec
finish create monitor integration
ajhollid Nov 26, 2024
101c5b7
Integrate into pagespeed configure
ajhollid Nov 26, 2024
93a77f4
integrate in to create pagespeed
ajhollid Nov 26, 2024
6443203
integrate in to settings
ajhollid Nov 26, 2024
16069b4
remove test route and page
ajhollid Nov 26, 2024
061b450
remove Field component
ajhollid Nov 26, 2024
c62d62f
Merge branch 'develop' into fix/fe/text-input-integration
ajhollid Nov 27, 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
6 changes: 0 additions & 6 deletions Client/src/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import { logger } from "./Utils/Logger"; // Import the logger
import { networkService } from "./main";
import { Infrastructure } from "./Pages/Infrastructure";
import InfrastructureDetails from "./Pages/Infrastructure/Details";
import Test from "./Pages/test";
function App() {
const AdminCheckedRegister = withAdminCheck(Register);
const MonitorsWithAdminProp = withAdminProp(Monitors);
Expand Down Expand Up @@ -90,11 +89,6 @@ function App() {
path="/"
element={<HomeLayout />}
>
<Route
path="/test"
element={<Test />}
/>

<Route
exact
path="/"
Expand Down
3 changes: 1 addition & 2 deletions Client/src/Components/Check/Check.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@ const Check = ({ text, noHighlightText, variant = "info", outlined = false }) =>
const theme = useTheme();
const colors = {
success: theme.palette.success.main,
error: theme.palette.error.contrastText,
error: theme.palette.error.main,
info: theme.palette.info.border,
};

return (
<Stack
direction="row"
Expand Down
50 changes: 0 additions & 50 deletions Client/src/Components/Inputs/Field/index.css
View file
Open in desktop

This file was deleted.

241 changes: 0 additions & 241 deletions Client/src/Components/Inputs/Field/index.jsx
View file
Open in desktop

This file was deleted.

9 changes: 7 additions & 2 deletions Client/src/Components/Inputs/TextInput/Adornments/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ export const HttpAdornment = ({ https }) => {
);
};

HttpAdornment.propTypes = {
https: PropTypes.bool.isRequired,
};

export const PasswordEndAdornment = ({ fieldType, setFieldType }) => {
const theme = useTheme();
return (
Expand All @@ -55,6 +59,7 @@ export const PasswordEndAdornment = ({ fieldType, setFieldType }) => {
);
};

HttpAdornment.propTypes = {
https: PropTypes.bool.isRequired,
PasswordEndAdornment.propTypes = {
fieldType: PropTypes.string,
setFieldType: PropTypes.func,
Comment on lines +62 to +64
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Yo dawg, we got some serious prop validation issues up in here! 🍝

Looking at the implementation and usage, there's a major issue that's making my knees weak:

  1. The component is being used without required props in test.jsx: <PasswordEndAdornment />
  2. The component's logic explicitly depends on both props:
    • fieldType is used in comparison: fieldType === "password"
    • setFieldType is called in the onClick handler

The current PropTypes are indeed looser than mom's spaghetti! Here's what's wrong:

  • Props are missing in actual usage
  • The component will throw runtime errors when clicking the button
  • fieldType is only valid as "password" or "text"

The original review comment is spot-on with its suggested fix:

fieldType: PropTypes.oneOf(['password', 'text']).isRequired,
setFieldType: PropTypes.func.isRequired,
🔗 Analysis chain

Yo, these PropTypes are looser than mom's spaghetti! 🍝

The current PropTypes declaration needs some tightening up to prevent potential runtime issues:

  1. Both props are essential for the component to function, so they should be marked as required
  2. fieldType should be restricted to specific values since it's used in a comparison

Here's how we can make it more robust:

 PasswordEndAdornment.propTypes = {
-	fieldType: PropTypes.string,
-	setFieldType: PropTypes.func,
+	fieldType: PropTypes.oneOf(['password', 'text']).isRequired,
+	setFieldType: PropTypes.func.isRequired,
 };

Let's verify if these props are always provided:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for PasswordEndAdornment usage to ensure props are always provided
rg -A 2 "PasswordEndAdornment"

Length of output: 1493

Script:

#!/bin/bash
# Let's check the implementation to see how fieldType is used
ast-grep --pattern 'const PasswordEndAdornment = ({ fieldType, setFieldType }) => {
  $$$
}'

# Also check the test file to see how it's being used
cat Client/src/Pages/test.jsx

Length of output: 7257

};
Loading