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

MyProfile - Validate input string for empty values #204

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

Conversation

prajwalkulkarni
Copy link

Issue Number

fixes #203

Describe the changes you've made

The handleInputChange callback is triggered on every keypress, also the user information is updated on every keypress and stored in the state variable user of the MyProfile component. However, when the save icon is pressed to update the information, no validation is done to check whether the field that was currently being updated is actually updated or not(Specifically, checking if any empty value has been passed or not). Skipping such validation results in unnecessary network calls. Hence, in the handleInputChange cb, I have set up a state updating function that updates the currently updating field on every keystroke. Thus, when the save icon is pressed, we can verify if the updated field holds any string or not(by checking its length). If it happens to be an empty string, an error toast is fired saying Input can not be empty. This ensures that the user actually enters a non-empty input.

Describe if there is any unusual behavior (Any Warning) of your code(Write NA if there isn't)
NA

Additional context (OPTIONAL)

Test plan (OPTIONAL)

A good test plan should give instructions that someone else can easily follow.

Navigate to My Profile (/myProfile).
Click on the pencil icon of any input fields.
Click on the save icon leaving the input field empty.
You can notice an error toast pop up.

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • The title of my pull request is a short description of the requested changes.

Provide a Deployed link of route/page that needs to review

Preview: Deploy preview link here with the appropriate route

@cyntss
Copy link
Member

cyntss commented Mar 29, 2022

The problem I see with this implementation is that it really isn't validating anything. Values are stored regardless of the toast notification saying that the input cannot be empty.

First, we need to question ourselves what are the use cases:

  • a user profile may have a twitter handle that needs to be removed, therefore, empty inputs are a way to remove data. In this case we should detect this and tell the user "your data has been removed"
  • a user may have nothing in the twitter field, and then s/he clicks to edit, but adds nothing, and simply clicks on save. Nothing changed, so nothing was removed. In this case we can respond "nothing happened here"
  • a user has a twitter user and wants to fix it because s/he made a mistake in the name. In this case, we respond "your changes have been saved"
  • ... maybe more scenarios, but I don't want to get away from the basic ones related to "empty VS non-empty"

Try to focus on that, and lets boost this PR

Copy link
Member

@cyntss cyntss left a comment

Choose a reason for hiding this comment

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

let's improve this PR focusing on the user experience described above

@prajwalkulkarni
Copy link
Author

There are two aspects that need to be considered here.

  1. User experience.
  2. Network calls.

The scenarios mentioned by @cyntss can be divided into either of the above-mentioned categories.
If there are any changes made(either updates or clearing the input field) only then the application should fire a network request with relevant toast messages. If no changes are made, an appropriate toast message can be displayed like, "nothing happened here" without making any network request because nothing has been changed. Sounds fair?

Consider the below as per the given scenarios:

Action Toast message Network call
Clear input field (E.g: remove Twitter handle) Your data has been removed
No changes made (Empty input field before & after update) Nothing happened here
Updated input field(Empty to non-empty) Your changes have been saved
No updates on the non-empty field (before update: ABC, after update: ABC) meaning the user had entered the edit mode but didn't really change anything Nothing happened here
Update input field(Non-empty to different non-empty value) Your changes have been saved

I think this covers most of the cases.

@prajwalkulkarni
Copy link
Author

prajwalkulkarni commented Mar 30, 2022

I've improved the PR that focuses on UX and displays relevant toast messages as per the actions depicted in the above table.
I've created a new function assignCurrField that drills down the component hierarchy to lift the state up for the input values. This is to track the initial value and the value after clicking the save icon, these two values are compared to determine if there have been any changes(updates, deletion) or not. Based on the comparison network calls and a suitable toast message is displayed. I've also considered corner cases where the user can open two or more inputs at a time, add value to one field, remove from the other, save another as it is. Under all the cases, appropriate messages are shown. I've also run lint tests and there are no errors.
@cyntss Kindly request you to review the changes and let me know what you think of it.

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.

Profile inputs accepting empty values
2 participants