Response and Commit to open fields position swaped in LT table #586
Conversation
|
|
|
I think you can remove "good first issue" from the title. 👍 |
|
Could you share a screenshot of you reproducing the issue by yourself and then the fix? Currently the issue screenshot is from the original reporter, and contains a different selection of columns and data which makes it a slightly less than a 1:1 comparison. In terms of the code change itself, I think it's sound and needs no further changes. |
sahitya-chandra
changed the title
|
@jayantbh This is a current screenshot of the issue
|
|
If that's a screenshot of the issue, what does the fix look like? |
|
The fix is just to swap response and commit to open fields in the table. |
|
I understand, but generally it's a good idea to show visual example of the fix. For example, what the issue looked like before, and how your change impacted it. :) It makes it easier for a reviewer to review and approve your PR, and improves trust in your PRs. 🚀 Given that the response times don't seem to exist in those PRs, you could sync another repo with PR activity, such as Middleware itself. |
|
Ok sir i will do it right away |
|
Previous After: Sir have a look |
|
It seems that the issue isn't completely resolved. You see how commits, lines, and comments are all showing time units even after your change? They should be some number values. Basically it seems like multiple headers aren't set in the correct order w.r.t. their body columns. You would need to address those as well to fully fix this. |
|
|
|
previously i was applying filter in configure options that's why the numbers were not showing
|
|
Oh that is strange, because the headers should also hide if you unselect the columns from configure columns. Is that not what's happening? |
|
No sir, when i unselect commits or lines their header keeps on showing |
|
@jayantbh Sir in 2nd commit I have fixed unhiding issue of column when unselected in configure columns. Screencast.from.2024-10-10.23-00-05.webm |
|
Ah, that's more like it. Thanks for sorting that out. |
|
sure sir |
|
Sir, linting done |
|
Hey again, so sorry this is one last trivial thing left. Could you undo changes to those files and push again? I think the rest is good to go. |
|
The linter still failed after your last change. Check the above checks. |
|
| @@ -67,7 +67,7 @@ export const PullRequestsTableHead: FC<PullRequestsTableHeadProps> = ({ | |||
| <Checkbox | |||
| checked={allPrsSelected} | |||
| indeterminate={somePrsSelected} | |||
| onChange={(e) => { | |||
| onChange={(e: any) => { | |||
There was a problem hiding this comment.
Don't use any.
Identify what the right type for this will be. Leverage the powers that Typescript offers.
There was a problem hiding this comment.
If I didn't miss this earlier, I'd have approved it this time.
There was a problem hiding this comment.
sir, I have added the correct type now
|
That's it! Done! Great work @sahitya-chandra! Hopefully your subsequent PRs will go smoother. :) |
Learn a lot this time |
resolved #547