GoogleAuth works!

[DemetrPI]

i18n translation added, Oauth2 via Google account added, multi-level menu, footer added, Auth Page added, Contact Us page added. Chakra rocks!

[abenteuerzeit]

Hey @DemetrPI

Thanks so much for your amazing efforts and wonderful ideas! I really appreciate all the energy you contribute to our project. I've spent some time reviewing your pull request #10 for Tenant Talk. I appreciate the effort you've put into this, and I have some feedback that I believe will help improve the quality of the code and align it more closely with our project's standards.

Changes

A total of 6 commits to the i18n branch and requesting to merge these changes into the development branch. Firstly, I want to commend you, @DemetrPI, for all the work you've done on this codebase. It's clear that you've put a lot of thought into these changes. I appreciate your effort in improving our app.

1. Implement 18n translation
2. Implement OAuth2 via Google account
3. Add a multi-level menu
4. Add a footer
5. Add an Auth Page
6. Add a Contact Us page

Commits

Let's go through each commit

1. Commit 2751f06:

this commit shows substantial progress in the development of the application. The addition of i18n and the mobile menu popup are significant improvements. However, there are a few areas, such as the .gitignore update and the size of the commit, which could be improved.

• Internationalization (i18n): adding i18n is a vast improvement to the project. The use of react-i18next is an excellent choice as it's a powerful internationalization framework for React. The instructions provided in the README.md file for using i18next-scanner is clear and helpful.
• Mobile Menu Popup: The implementation of the mobile menu popup in Navbar.js is well done. The use of the useState hook to manage the state of the menu (open or closed) is a good practice. The SVGs used for the menu icon and the close icon are appropriate and their visibility is correctly controlled based on the state of the menu.
• MongoDB Dependency: there's no implementation related to MongoDB in this commit. It would be better to include the MongoDB related changes in the commit where it's actually used.
• **.gitignore **: The addition of mongodb://localhost:27017 and localhost:27017 to the .gitignore file seems unnecessary as these don't appear to be related to any file or directory that should be ignored. It's important to only include relevant entries in the .gitignore file.
• Commit Size: This commit includes many changes (over 14,000). While it's understandable that some commits may include many changes due to the nature of the work being done, it's generally a good practice to try to keep commits smaller and more focused. This makes it easier to understand the specific changes that were made and why.

2. Commit 641a9d8

The commit seems to be a significant one, introducing multiple new features. It would be beneficial to break down such large commits into smaller ones in the future, each handling a single logical change. This makes it easier to understand the changes and to identify the source of any bugs that arise.

-Commit Message: The commit message is clear and descriptive. It mentions the main features added - i18n, contact form, login form, footer, header. it would be more helpful if the commit message also included a brief description of how these changes impact the project or why they were necessary.

• Code Organization: The code seems to be well-organized. The changes are spread across multiple files, which is a good practice as it helps in maintaining the code and making it more readable.
• Package.json: The new dependencies added are relevant to the features implemented. it's important to ensure that these new dependencies do not introduce any security vulnerabilities. use a tool like npm audit to check for any potential security issues.
• App.js: The changes in this file are consistent with the features added. The use of ChakraProvider is appropriate for using Chakra UI components. it would be better if the import statements were organized, perhaps grouping external and internal imports separately for better readability.
• Confetti.jsx: This new file seems to be handling some styling related to confetti. It would be helpful to have some comments explaining what the various parts of the code are doing, especially for future reference or other developers working on the project.
• tenant_frontend/src/assests/images/Confetti.jsx: There seems to be a typo in the directory name 'assests'. It should be 'assets'. This could lead to confusion in the future.
• Code Quality: The code quality seems to be good.
• Testing: There's no indication of any new tests being added alongside these changes. the project should include tests, it's important to update them or add new ones to cover the new features.

3. Commit dbb843e

The refactoring of the i18n implementation is generally well done. The changes are consistent across all the files and the original approach of using a useTranslatedItems hook to handle translations is a good practice. This approach centralizes the translation logic and makes the code more maintainable and scalable

MenuTexts.jsx The refactoring in this file is excellent. The use of the useTranslation hook within the useTranslatedItems custom hook is a good practice. It makes the code cleaner and more readable. The translation of the menu items is now done in one place, which is more efficient and easier to maintain.

ChakraFooter.jsx DesktopNav.jsx, MobileNav.jsx UserNav.jsx: The changes in these files are consistent and correct. The use of the useTranslatedItems hook simplifies the code and makes it more readable. The removal of the t function from the useTranslation hook and the direct use of the translated strings is a good practice. It makes the code cleaner and reduces redundancy.

trans.json: The correction of the typo from "progects" to "projects" is a good catch. It's important to ensure that the translation files are accurate and free of typos.

Suggestions for Improvement
-Null Checking In the files where you map over the items returned from useTranslatedItems, you've added a check to ensure the items exist before mapping over them (e.g., NAV_ITEMS && NAV_ITEMS.map(...)). This is a good practice to prevent potential errors if the items are undefined or null. since you're already defining these items in MenuTexts.jsx, this check might not be necessary unless there's a chance, they could be undefined or null. If there's no such chance, you could remove these checks to simplify the code.
-Translation Keys: In the MenuTexts.jsx file, you're using the t function to translate the keys directly. This might lead to issues if the keys in the translation files change. A better approach would be to use the keys as they are and only use the t function in the components where these keys are used. This would ensure that any changes in the translation files would not affect the rest of the code.
-Code Comments: While your code is generally clean and easy to understand, adding more comments explaining the purpose of the functions and components, as well as the changes you've made, would be beneficial for other developers who might work on this code in the future.

4. Commit 04fdc5f

The changes made in this commit are minor but crucial for the correct functioning of the application. The custom hook useTranslatedItems is now being invoked correctly in all the components. This is a good catch and an important fix.

ChakraFooter.jsx, DesktopNav.jsx, UserNav.jsx: In these files, you've corrected the usage of the useTranslatedItems hook. Previously, it was being used without the parentheses, which would not invoke the function and would instead return the function definition. This would cause errors when trying to destructure the returned values. Now, you're correctly invoking the hook with parentheses, which should return the expected values and allow for correct destructuring. This is a crucial fix and is correctly implemented.

Suggestions for Improvement
-Commit Message While the commit message "Minor bug fix in custom hook declaration" is accurate, it could be more descriptive. A more detailed message like "Fixed incorrect invocation of useTranslatedItems hook in various components" would provide more context about what the bug was and where it was fixed. See the commit writing style guide cheat sheet here => #9
-Code Comments: It would be beneficial to add comments in the code explaining why this change was necessary, especially since the difference between the correct and incorrect code is subtle and could be easily overlooked by other developers.

5. Commit 414bc6a

introduces several key features to the application, including Google authentication, additional internationalization, and email form validation. The changes are well-structured and seem to follow good practices.

• package.json: Two new dependencies, react-social-login-buttons and reactjs-social-login, have been added. These are necessary for the Google authentication feature. It's good to see that the versions are pinned to avoid potential future compatibility issues.
• manifest.json: The sizes of the icons have been changed. This is a minor change but it's important to keep the manifest file up to date.
• App.js The import statements have been updated to reflect the new components and routes. The changes are consistent with the rest of the codebase.
• ChakraSocialAuth.jsx: This new component handles Google authentication. The code is well-structured, and the use of hooks and callbacks is appropriate. The use of the useTranslation hook indicates that internationalization has been considered in this component.

Suggestions for Improvement

• Environment Variables: In ChakraSocialAuth.jsx, the Google client ID is fetched from the environment variables. It's good to see sensitive information being stored securely. it would be beneficial to handle the case where the environment variable is not set more gracefully. Currently, if the environment variable is not set, an empty string is used, which could lead to confusing errors. Consider throwing an error or showing a warning if the necessary environment variables are not set.
• Error Handling In the LoginSocialGoogle component, the onReject prop is used to log errors to the console. While this is fine for development, in a production environment, it might be better to show the user a meaningful error message or to send the error to an error tracking service.
  -Code Comments While the code is generally clear, adding comments explaining the more complex parts of the code, such as the authentication process, could be beneficial for other developers working on this codebase.

6. Commit 39ad424

This commit introduces Google Authentication and updates the internationalization (i18n) of the application. It also includes changes to form validation and user state management.

tenant_frontend/src/App.js
The UserProvider is now wrapping the entire application, which is a good practice for providing user context to all components. This change will allow all components to access the user state and update it as needed.

tenant_frontend/src/components/ChakraSocialAuth.jsx
The Google Authentication component has been refactored to use the UserProvider context. This is a good change as it centralizes the user state management. The user data is now stored in local storage, which will persist the user's login state across sessions. storing sensitive user data in local storage can have security implications. It would be better to store the data in HTTP-only cookies to prevent potential cross-site scripting (XSS) attacks.

tenant_frontend/src/components/auth_components/userProvider.jsx
This new component provides a context for user state. It uses the useContext and useState hooks to create a global state for the user. It also uses the useEffect hook to load and save the user data from and to local storage. This is a good practice for managing global state in a React application.

tenant_frontend/src/components/header_components/MenuTexts.jsx and tenant_frontend/src/components/header_components/UserNav.jsx
These components have been updated to use the UserProvider context. This is a good change as it allows these components to access and update the user state.

tenant_frontend/src/pages/LogInCard.jsx
The form validation has been updated to require the password field. This is a good change as it improves the security of the login form.

tenant_frontend/src/routes/AllRoutes.js
The routing has been updated to include error handling. This is a good change as it improves the user experience by providing feedback when a route is not found.

tenant_frontend/src/utils/locales/en/trans.json, tenant_frontend/src/utils/locales/pl/trans.json, tenant_frontend/src/utils/locales/uk/trans.json
These files have been updated to include translations for new text introduced in this commit. This is a good practice for maintaining the internationalization of the application.

Suggestions

1. Consider storing user data in HTTP-only cookies instead of local storage to improve security.
2. Ensure that all sensitive user data is properly encrypted before storing it.
3. Consider adding tests for the new functionality introduced in this commit.
4. Ensure that all components that use the UserProvider context handle the case where the user data is not available.
5. Consider adding error handling for the Google Authentication process.

File Changes

Looking at the file changes, it's clear that you've put a lot of thought into the structure and organization of your code. I noticed that there are a few areas where error handling could be improved. For example, in the user registration and password update features, it would be beneficial to add some error handling to account for potential issues.

Pull Request

Splendid work on this pull request. Your code is clean and well-structured, and it's clear that you've put a lot of thought into these changes. Keep up the excellent work!

• i18n Integration: The integration of i18n is a significant step towards making our application more accessible to users worldwide. However, I noticed that the i18n scanner configuration file is in the src directory (src/i18next-scanner.config.js). It would be more appropriate to move this file to a config directory at the root level of the project for better organization.
• Google OAuth: The addition of Google OAuth is an excellent feature for our users. However, I couldn't find any error handling for failed authentication attempts. It would be beneficial to add some error handling to improve the user experience in case of authentication failure.
• Code Organization: I noticed that the ContactUs.js and ContactUs.jsx files exist. If these files serve the same purpose, it would be best to stick to a single file type for consistency. I suggest we stick to the .jsx extension for React components.
• Commit Messages: Your commit messages are descriptive, which is excellent. However, they could be more atomic. For example, the commit "Added i18n, contact form, login form, footer, header." could be split into separate commits for each feature added. This approach makes it easier to understand the history and isolate any potential issues in the future.
• Dependencies: I checked the project's dependencies and noticed that some packages might not be in use, such as @adobe/css-tools. Please ensure to remove any unused dependencies to keep the project clean and efficient.
• Code Documentation: While your code is generally clean and easy to follow, there are places where additional comments could be helpful, especially around the Google OAuth and i18n implementation. This will help other team members understand your code more quickly and maintain it in the future.
• Pull Request Description: Lastly, while your PR description provides a good overview of what's been done, it would be helpful to include more details about how these changes affect the application's functionality and any new libraries or dependencies introduced.

Best,
@abenteuerzeit

[abenteuerzeit]

i'll resolve the merge conflicts and merge the branch. hang tight D!