Skip to content

Conversation

@Tamir198
Copy link
Member

Creating this pr for noam for cr

@vercel
Copy link

vercel bot commented Oct 13, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
colman-dev-club Error Error Oct 18, 2025 9:44am
website Error Error Oct 18, 2025 9:44am

💡 Enable Vercel Agent with $100 free credit for automated AI reviews



export let analytics;
if (typeof window !== 'undefined') {
Copy link
Member Author

Choose a reason for hiding this comment

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

Try to convert this code into async await just like we are doing inside the entire code base

// validator: passwordValidation,
// },
{
label: "Password",
Copy link
Member Author

Choose a reason for hiding this comment

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

Check with one of the other mentors that this is ok to un-comment

return parsedData;
} catch (error) {}
const parsed = parseCsv(csvData);
console.log('[useSheets] parsed rows:', parsed.length, parsed.slice(0, 3));
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove the console

console.log('[useSheets] parsed rows:', parsed.length, parsed.slice(0, 3));
return parsed;
} catch (error) {
console.error('[useSheets] fetch failed:', error);
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove the console


const parseCsv = (csvData) => {
return csvData.split('\n').map((row) => row.split(/","/));
return csvData
Copy link
Member Author

Choose a reason for hiding this comment

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

Check if we can parse the file something like this :

return data.split(/n).map((row) => row.split(/","/)

queryKey: ['googleSheetsData'],
queryFn: fetchDataFromCsv,
refetchOnWindowFocus: false,
staleTime: 5 * 60 * 1000,
Copy link
Member Author

@Tamir198 Tamir198 Oct 13, 2025

Choose a reason for hiding this comment

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

The magic number should be saved inside a const variable

For example:

const VARIABLE_NAME = 5

And so on

import Avatar from '@mui/material/Avatar';
import { useCreateUser } from 'src/hooks/firebase.hooks';

// ✅ ייבוא ישיר לבדיקה יעילה ב-Firestore
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove


const createUser = useCreateUser();

const onSignupHandler = async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Check if you can push your changed into smaller helper function

if (!rules) return;

try {
const emailToCheck = String(formValues.email || '').trim().toLowerCase();
Copy link
Member Author

Choose a reason for hiding this comment

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

use existing validations, also the formValues should already contain valid email because otherwise we cannot validate the form - check it

if (!emailToCheck) return;

if (validationState[key]) {
const q = query(
Copy link
Member Author

Choose a reason for hiding this comment

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

This entire part could be extracted into a service that handles all of the users.

paddingBottom: '3rem',

}}
sx={{ paddingTop: '3rem', paddingBottom: '3rem' }}
Copy link
Member Author

Choose a reason for hiding this comment

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

Generally its not recommended to mix inline style into the jsx.
You can use external file to save those styles.

})}
</Box>

{/* מודל הצלחה */}
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove comment

fieldOfStudy: '',
});

const setField = (key) => (e) => setForm((p) => ({ ...p, [key]: e.target.value }));
Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think we can have a better name for this ? p is a bit confusing in this context

Comment on lines +38 to +42
const q = query(
collection(db, COLLECTION),
where('email', '==', normalized.email),
limit(1)
);
Copy link
Member Author

Choose a reason for hiding this comment

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

This should come from the user service that we talked about before

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.

3 participants