restricted-site-access
restricted-site-access copied to clipboard
Rename button and add empty validation shake
Description of the Change
Renamed the Add button to Add this IP and added an animation when the Add this IP button clicked without an IP added.
Closes #140
Verification Process
- Visit the general settings page
- Under
Unrestricted IP addressessection we should see theAdd this IPbutton - When clicking on the button without adding an IP, IP text box should shake
- When clicking on the button with the right IP, it should get added
Checklist:
- [x] I have read the CONTRIBUTING document.
- [x] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I have added tests to cover my change.
- [x] All new and existing tests passed.
Hi @dhanendran, Tung has suggested in the issue comments to proceed with the 2nd method where if there's just one IP, then clicking on Save must save it without having the need to click the "Add" button. Can you please update the PR to accommodate this change? Thanks in advance!
Hi @dhanendran, Tung has suggested in the issue comments to proceed with the 2nd method where if there's just one IP, then clicking on Save must save it without having the need to click the "Add" button. Can you please update the PR to accommodate this change? Thanks in advance!
@Sidsector9 At the moment there are few validations happening on the Add button click like duplicate validation, IP format validation which we need to completely rewrite the Add and Save functionality if we are going with the 2nd option. Also, since we are whitelisting the IP and providing unrestricted access having to click on the Add button just to be extra cautious makes sense to me. That's why I went with the option 1. Happy to discuss if I missed any other important point.
@dhanendran Thanks for working on this PR and explanation!
At the moment there are few validations happening on the Add button click like duplicate validation, IP format validation which we need to completely rewrite the Add and Save functionality if we are going with the 2nd option.
I see your point. We're using JS to create inputs for IPs and comments. So we need to rewrite some parts of the settings.js file if we want to go with the 2nd option.
But still, this PR doesn't fully resolve #140. The use case when users input the IP and then click the Save changes button is common and natural IMO. I can think of some directions:
- Listen to the submit event, if the IP field isn't empty, hold the submission, click the Add this IP button then submit the form.
- Refactor the setting page to support saving the IP without adding it to the list.
- Add a message below the IP input field stating that users need to click the Add this IP button. The message should appear only when users start inputting the IP to catch their attention.
I think the first direction is error-prone so we shouldn't go with it. The second requires more effort but it fully solves the issue. The last direction is the safest and quickest one.
I'd suggest the 2nd one, but it's totally up to you to decide what to do next.
Hi @dhanendran just following up. Do you have any thoughts on Tung's comment?
@Sidsector9 I will work on it this week.
@Sidsector9 pinging you again for re-review
@Sidsector9 you can take over as I won't be able to work on this for next couple of weeks.