Closed PC25 closed 3 years ago
Can we remove accordions in the case when there is no child?
The subscription view is paginated, which I don't think is desired?
Pass a key prop while using the map
Please configure your VS Code according to this https://app.nuclino.com/IMGIITRoorkee/Omniport-Development/Frontend-623e56dd-54c2-428e-93e0-a52f2c030d89 and there won't be any deviation from the Omniport frontend code conventions. Once configure your VS Code according to this and press Ctrl+S on every component you edited and push that code.
@PC25 Really nice progress :tada:, it is very near to the merge. I have just a few concerns, please address those.
[x] Currently, if I am already subscribed and make a subscribe request again, which is happening whenever using the "Manage subscriptions" second time as from the frontend it sends the keys again. So, please handle the duplicate key case, currently, it throws a 500.
[x] Also you have used directly .get
there if key doesn't exist it will raise an error, that should be handled too.
[x] If I uncheck a sub-category, parent category is not being un-subscribed. The overall handling of this tree seems really fuzzy. I understand you are being busy with your tests and there is a lot of it going. But this carries no less importance also. You may take help of your mates in that case. @utkarshparkhi if you can have a look at it with @PC25
@gouranshi Please suggest for the following potential UI issues.
[ ] UI: Add a toast with the success message too, as sometimes person is in lower screen so he is unable to see the message above the form
[x] UI: If any sub-category is subscriber, there should be some indication in the main category
@PC25 @utkarshparkhi really nice work. It's working as expected. Will do a code review now. :tada:
Just the success/error toast message is pending. And currently, to check if a parent is in an intermediate state, you are checking If any of its immediate children are in checked state, but the right case is to check if any of its immediate children are in an intermediate state or checked state.
Mentioning the other related PRs to make it easy for reviewing: Notifications view: https://github.com/IMGIITRoorkee/omniport-service-notifications/pull/11 Emails view: https://github.com/IMGIITRoorkee/omniport-service-emails/pull/12 Fix subscription tree serializer: https://github.com/IMGIITRoorkee/omniport-service-categories/pull/2
@Ajayneethikannan @shreyansh23 Requesting you to once go ensure the code conventions and quality also. YOu may also test the functionality but I have gone through it and suggested from it once.
@PC25 Quite fast and excellent work on this PR. The code seems good except few console statements. Will review it by the weekend.