-
-
Notifications
You must be signed in to change notification settings - Fork 759
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
Implementation of sorting of requests in Request screen #1082
Implementation of sorting of requests in Request screen #1082
Conversation
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersWhen your PR has been assigned reviewers contact them to get your code reviewed and approved via:
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1082 +/- ##
===========================================
+ Coverage 92.77% 94.64% +1.87%
===========================================
Files 134 138 +4
Lines 3238 3324 +86
Branches 904 922 +18
===========================================
+ Hits 3004 3146 +142
+ Misses 225 171 -54
+ Partials 9 7 -2 ☔ View full report in Codecov by Sentry. |
Thanks. Please also get the test code coverage for the typescript files you edited up to 100% We are trying to get back up to 95% coverage overall to ensure the reliability of our code. |
|
Good work @chandel-aman, please complete the points that I have asked in the review. |
@rishav-jha-mech Your review is not visible. Please put a comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
@rishav-jha-mech Could you please comment down the changes that needs to be done? |
@palisadoes @rishav-jha-mech |
src/screens/Requests/Requests.tsx
Outdated
<Dropdown.Toggle | ||
variant="outline-success" | ||
data-testid="sortuser" | ||
> | ||
<SortIcon className={'me-1'} /> | ||
{t('sort')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here instead of Sort
text you should show the selected option, and the sort button should become of variant='success'
meaning sorting is active now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Dropdown.Item href="#/action-3">Action 3</Dropdown.Item> | ||
<Dropdown.Item | ||
onClick={(): void => handleSorting('latest')} | ||
data-testid="latest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whichever option is selected should be highlighted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/screens/Requests/Requests.tsx
Outdated
@@ -372,6 +418,11 @@ const Requests = (): JSX.Element => { | |||
</Table> | |||
</InfiniteScroll> | |||
)} | |||
{!isLoading && displayedUsers.length === 0 && ( | |||
<div className="w-100 text-center my-4"> | |||
<h5 className="m-0 ">{t('noRequestFound')}</h5> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this line ?
{t('noResultsFoundFor')} "{searchByName}"
If no request is found for a particular search, the above text should be displayed
Also @chandel-aman you may add time of the request in the table column, and it should be of humanize format, ex 1 Day ago, 4 hours ago, 1 year ago, etc. |
@rishav-jha-mech Thanks for your review. I'll make the changes that you've suggested. |
@rishav-jha-mech This is not related to this issue of sorting the requests, should I make a separate issue for this? Also, we only have date for when the user was created not date when the request was made. So do we have to display the user created date in requests? |
@chandel-aman then that means that you are sorting on basis of user creation, so add that field in column for now, humanize the date time. |
@rishav-jha-mech Sure, will do that! |
@chandel-aman Any update on this? Please get this PR merged, its been more than 2 weeks. |
@noman2002 I've made all the changes asked in the review. Was facing some issue in test coverage of request screen. I'm working on it; will try getting this PR merged asap. |
@rishav-jha-mech @noman2002 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM |
@palisadoes this can be merged |
What kind of change does this PR introduce?
Bug
Issue Number:
Fixes #1073
Did you add tests for your changes?
Yes
Snapshots/Videos:
www_screencapture_com_2023-11-22_02_37.webm
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
Yes