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

Mn.approve reject modal2 #363

Merged
merged 36 commits into from
Jul 2, 2024
Merged

Mn.approve reject modal2 #363

merged 36 commits into from
Jul 2, 2024

Conversation

maxn990
Copy link
Contributor

@maxn990 maxn990 commented Jun 2, 2024

Checklist

  • 1. Run yarn run check
  • 2. Run yarn run test

#360

maxn990 and others added 30 commits March 26, 2024 19:38
Finished most of the UI for the grid view and the modals
Copy link
Contributor

@huang0h huang0h left a comment

Choose a reason for hiding this comment

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

💯 awesome work! A couple small comments about the code, and a couple comments about the UX:

Comment on lines 206 to 208
>
Accept
</Button>
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ Seems like this button's contents got deleted, which causes the "Accept" button to not show on the list view:

image

Comment on lines 150 to 162
const filteredData = useMemo(
() =>
fetchData
? fetchData.filter(
(data) => !approvedOrRejectedImageIds.includes(data.imageId),
)
: [],
[fetchData, approvedOrRejectedImageIds],
);

const tableData = useMemo(
() => (fetchData ? fetchData.map(responseToTableData) : []),
[fetchData],
() => (filteredData ? filteredData.map(responseToTableData) : []),
[filteredData],
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ What's the reason for putting filteredData into its own useMemo here? Seems like we only use it to compute tableData unless I'm missing something

Comment on lines 114 to 117
toReject.push(protectedApiClient.rejectImage(data.key, rejectionReason));
Promise.all(toReject).then(() => {
props.setApprovedOrRejectedImageIds((prevIds) => [...prevIds, data.key]);
close();
Copy link
Contributor

@huang0h huang0h Jun 3, 2024

Choose a reason for hiding this comment

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

⛏️ Can we add some feedback with the message antd component (specifically, message.success() on success, and message.error() on failure) once the API call finishes? That way, the user can know for sure the outcome of their action instead of having to assume it went through fine.

Comment on lines 127 to 130
Promise.all(toApprove).then(() => {
props.setApprovedOrRejectedImageIds((prevIds) => [...prevIds, data.key]);
close();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ Same as above - can we add some user feedback here with message as well?

);
}

const TreeSummaryDisplay = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ Right now, if someone selects an image on the list view and then switches to the grid view, it's not super clear which ones are selected:

image

Could we add some visuals to show which images are currently selected? Some things I'm thinking are:

  • green border around the image
  • something like in the figma where a checkmark is overlaid: (although I worry a little it might get read as "approved" instead of "selected")
    image

though if you think of something you feel is better, definitely go for it!

};

const GridView = () => {
const pageSize = 9; // Number of cards per page
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want the pageSize to stay constant 9 or scale with viewport size?

() =>
fetchData
? fetchData.filter(
(data) => !approvedOrRejectedImageIds.includes(data.imageId),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure I understand, whenever someone accepts or rejects an image, after closing the approval box modal the images should disappear from grid view without refreshing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I might just be missing it, but in table view does approving/rejecting also update this approvedOrRejectedImageIds list? If not, then the changes made in table view might not be reflected in grid view if not refreshed.

@CerberusLatrans
Copy link
Contributor

Great job on this absolutely behemoth of a ticket guys! very impressive. It was a nightmare to read through

@huang0h huang0h merged commit 7bda8d8 into master Jul 2, 2024
2 of 5 checks passed
@huang0h huang0h deleted the MN.Approve-Reject-Modal2 branch July 2, 2024 02:05
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.

3 participants