-
Notifications
You must be signed in to change notification settings - Fork 30
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
Homepage: Create RecentProjects component, refactor data fetching in RecentSubjects #6125
Conversation
// Returns all projects a user has classified in descending order (most classifications first) | ||
const { data: statsData } = useStats({ | ||
sourceId: authUser.id, | ||
query: { project_contributions: true } | ||
}) |
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.
@mcbouslog @seanmiller26 as mentioned in the Slack thread, the response from this is a list of projects in descending order from greatest number of classifications to least number of classifications by the user.
Is this "Continue Classifying" section intended to be "projects you've most recently classified"? If so, I'll need to change this fetch to a user's project preferences like PFE's homepage.
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.
Yes, this is a recently classified section, so we can match PFE here.
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.
Sounds good - I refactored to display projects a user has most recently classified on. The catch with relying on project_preferences
is that project_preferences.updated_at
is changed when a user interacts with a project beyond just classifying. For instance, closing a tutorial.
In this PR, I only fetched 1 page of project_preferences
(20 items) and filtered for those where activity_count > 0
(which means a user has actually classified on a project). This means if I recently interacted with 20 projects, but only classified on 3, then only 3 projects will show in this "Continue Classifying" UI section.
It would be much smoother if the eras request for project_contributions
included a sort by most recently classified, but I'll have to follow-up with Michelle after internal team testing.
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.
A clarification - PFE's homepage recursively gets ALL pages of a user's project preferences rather than just 1 page. This is why it takes a long time for all of your ribbon classifications to load. I'd prefer not to use recursion on the new homepage because it's so data intensive - hence the note about follow-up with Michelle and eras.
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.
I did not know the specifics here re project_preferences.updated_at
Great decisions!
packages/lib-user/src/components/UserHome/components/RecentSubjects/RecentSubjects.js
Show resolved
Hide resolved
? recents.slice(0, 10).map(classification => { | ||
const subjectMedia = classification.locations.map( | ||
? recents.map(recent => { | ||
const subjectMedia = recent?.locations?.map( |
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.
Changing this variable name to recent
instead of classification
was suggested here, because the object is a Recent resource from panoptes, not a Classification resource typically sent from lib-classifier to panoptes.
packages/lib-user/src/components/UserHome/components/RecentSubjects/RecentSubjectsContainer.js
Show resolved
Hide resolved
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.
Looking good!
I made a few changes, see in-line comments. Might be helpful to review last few commits and make sure changes are ok, I might be missing some context.
@@ -30,7 +30,7 @@ function CardsRow({ children }) { | |||
as='ul' | |||
direction='row' | |||
gap='small' | |||
pad={{ horizontal: 'xxsmall', bottom: 'xsmall' }} | |||
pad={{ horizontal: 'xxsmall', bottom: 'xsmall', top: 'xxsmall' }} |
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.
Added padding top: 'xxsmall'
to ul
s that list cards (project, subject) to leave a little room for the focus styling.
</StyledBox> | ||
</StyledAnchor> | ||
<StyledBox | ||
forwardedAs='a' |
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.
This refactor gives the SubjectCard focus styling.
{!isLoading && error && ( | ||
<Box fill justify='center' align='center' pad='medium'> | ||
<SpacedText> | ||
There was an error fetching your recent projects |
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.
For consideration, show error.message
here
margin='0' | ||
> | ||
{projectPreferences.map(preference => ( | ||
<li key={preference?.project?.id}> |
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.
Added <li>
{!isLoading && | ||
projectPreferences?.length ? ( | ||
<Box | ||
as='ul' |
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.
Moved ul
after loading/error/empty states content
{!isLoading && recents?.length | ||
? ( | ||
<Box | ||
as='ul' |
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.
Moved ul
after loading/error/empty states content
mediaSrc={subjectMedia[0]} | ||
projectSlug={classification.slug} | ||
/> | ||
<li key={recent?.id}> |
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.
Added <li>
return [] | ||
} catch (error) { | ||
console.error(error) | ||
throw error |
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.
Since the related components handle error states I think this fetch should throw an error if it has one so that error can be included in the error
returned from useSWR
on line 51?
async function fetchUserRecents({ userId }) { | ||
try { | ||
const query = { | ||
page_size: 10, |
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.
Refactored to page_size: 10
as I think we only need 10
return response.body?.recents | ||
} catch (error) { | ||
console.error(error) | ||
throw error |
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.
Similar to RecentProjectsContainer, since the related components handle error states I think this fetch should throw an error if it has one so that error can be included in the error returned from useSWR on line 35?
Package
lib-user
lib-react-components
Linked Issue and/or Talk Post
Follows #6117
The rest of the Dashboard on the signed-in homepage is in #6121
Figma Design
Describe your changes
project_contributions
. See more comments about it below.badge
option to ProjectCard so a user's classification count can be displayed per card on the homepage.usePanoptesProjects
hook.How to Review
projects
array or an error.Checklist
PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.
General
yarn panic && yarn bootstrap
ordocker-compose up --build
and FEM works as expectedGeneral UX
Example Staging Project: i-fancy-cats