-
Notifications
You must be signed in to change notification settings - Fork 2k
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
User Profile: List component #98225
User Profile: List component #98225
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~85 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~1746 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
client/state/reader/lists/reducer.js
Outdated
return Object.assign( {}, state, keyBy( action.lists, 'ID' ) ); | ||
case READER_LIST_REQUEST_SUCCESS: | ||
case READER_LIST_UPDATE_SUCCESS: | ||
return Object.assign( {}, state, keyBy( [ action.data.list ], 'ID' ) ); | ||
case READER_LIST_DELETE: | ||
if ( ! ( action.listId in state ) ) { | ||
return state; | ||
} | ||
return omit( state, action.listId ); |
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.
Do we no longer need these cases?
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.
Overall, I think there may be obsolete lists
functionality in here confounding the situation.
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 change isn't necessary for this PR and deals with existing functionality, so I reverted it.
As we continue working on the Lists feature, we can determine later whether it makes sense to refactor.
userSlug: action.userSlug, | ||
lists: apiResponse?.lists, | ||
} ), | ||
onError: () => noop, |
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.
Should we have an error state? 🤔
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 think an error state could be handled as part of a separate PR.
1069ade
to
8888f2a
Compare
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
client/reader/user-stream/style.scss
Outdated
.user-profile__lists-body { | ||
max-width: 768px; | ||
margin: 0 auto; | ||
|
||
.user-profile__lists .reader-post-card__title { | ||
margin-top: 0; | ||
|
||
.reader-post-card__title-link { | ||
font-size: 1.25rem; | ||
} | ||
} | ||
} |
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.
Just realized I can probably clean up these class names a bit. Will do that tomorrow.
/> | ||
<div className="user-profile__lists-body"> | ||
{ lists.map( ( list: List ) => ( | ||
<div className="card reader-post-card is-compact is-clickable" key={ list.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.
<div className="card reader-post-card is-compact is-clickable" key={ list.ID }> | |
<div className="card reader-post-card is-compact" key={ list.ID }> |
This is-clickable
class makes the entire card look clickable with the cursor, but only the List title is actually a link. We should probably remove is-clickable
here or make the whole card a link. What do you think?
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 made the whole card a link! I think that experience is clearer.
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 left a comment about the clickable area. Also, we could probably improve the mobile view. But overall this is working and looks good to me. I think we can ship it and iterate on it.
Related to https://github.com/Automattic/loop/issues/271
Proposed Changes
This PR adds a component containing a user's public lists on the profile page.
Why are these changes being made?
Part of an overall project to create a user profile page.
Testing Instructions
/v1/read/lists/{user_slug}
endpoint used in this PR)/read/users/{user_id}/lists
Pre-merge Checklist