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.

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

Dev/additional settings #323

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Dev/additional settings #323

wants to merge 5 commits into from

Conversation

mAkeddar
Copy link
Contributor

user can change first last name in my account page
Phone number is only displayed for now
Add cropping to profile picture

Known limitations:

If user created account with 2 first names, the display will be wrong

Copy link

reviewpad bot commented Dec 18, 2023

AI-Generated Summary: This pull request adds display features for first name, last name, and phone number fields in the user profile section. Furthermore, it includes an image cropping functionality for the profile picture. Changes have been made in the 'package-lock.json', 'package.json', and 'MyAccount.tsx' files, with the latter being within the 'Profile' folder. Additional packages such as 'ImgCrop' from 'antd-img-crop' have been included. The language translation files for English, French, and German have been updated to add translations for 'First Name' and 'Last Name'. The package-lock.json file has been updated with new dependencies, their versions and hashes.

Copy link
Contributor

@mischakolbe mischakolbe 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 overall, marking as "Request changes" because of the question about splitting the names in our database already.

@@ -21,17 +23,37 @@ const MyAccount: React.FC = () => {

useEffect(() => {
if (user) {
setName(user.displayName);
if (user.displayName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I see this correctly, you're dealing with the names stored in the previous format (before the first/last name split) by splitting them on the fly.

Would it make sense to "bake" this process into our database? Essentially going through all names we currently have, split them and store them correctly? Tagging @hugo2410 as well, since it might relate to his territory ^^

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe in the previous set up, the user should have written FirstName followed by the last name, so ideally the outcome should be similar. I tried to check on the DB but i can't see it directly...

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the super delayed response - I hope you had an awesome Christmas! :PP

Totally; I just wonder whether we want to split those names from the previous setup (stored as displayName) and run a process over our database, which splits those names in the same way, but then stores the result as firstName, lastName into the DB, so all users are stored the same way and we don't have to have an if/else when fetching the names.

That way we don't have to worry about it again, for example if we want to use the name in other areas.

Copy link

reviewpad bot commented Dec 21, 2023

AI-Generated Summary: This pull request consists of two patches. The first patch has changes across multiple files, while the second one just modifies the src/Pages/Profile/MyAccount.tsx file.

The first patch makes significant updates to the user profile page where the following enhancements are observed:

  • The profile picture upload process is improved by adding cropping functionalities. This is facilitated by the new module 'antd-img-crop' version 4.18.0 which is added in the package.json and package-lock.json files.
  • The user's first name and last name are separately displayed instead of a single name.
  • There's also the addition of phone number retrieval and display.
  • The translations for English, French, and German languages are updated in the translation.json files for each language to properly display "First Name" and "Last Name" instead of the previous "Name".

The second patch removes an unnecessary debug console.log statement from the src/Pages/Profile/MyAccount.tsx file. Specifically, it removes the logging line for 'First Name'.

Copy link

reviewpad bot commented Dec 21, 2023

AI-Generated Summary: This pull request contains three patches.

The first patch introduces the ability to display the user's first name, last name, and phone number on the profile page. It adds the ability to crop the profile picture and also updates the localization files to use "First Name" and "Last Name" instead of "Name". This patch equips the 'antd-img-crop' library, modifies 'MyAccount.tsx' to handle first name and last name separately, and adjusts their respective translations in English, French, and German.

The second and third patches are minor amendments to 'MyAccount.tsx' that remove two console logs. These logs used to output "First Name" and "Last Name" when a user updated their name on the profile page. These changes clean up the code, removing log outputs that are no longer needed.

Copy link

reviewpad bot commented Dec 21, 2023

AI-Generated Summary: This pull request involves changes in the user profile management section of a web application. The patch includes updates to the package.json and package-lock.json, adding a new library 'antd-img-crop' for image cropping.

Moreover, changes are also made to 'MyAccount.tsx' to include the ability to display and modify the user's first and last name separately, rather than displaying a single name. The handling of the user's profile picture is adjusted to incorporate image cropping, and console logs are removed to clean up the code.

Translation files ('translation.json') have been updated accordingly in English, French, and German to account for the separation of first and last names.

In patches 2 to 4, console log statements have been removed for tidying up.

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

Successfully merging this pull request may close these issues.

3 participants