-
Notifications
You must be signed in to change notification settings - Fork 0
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
Create dynamic lists from Explore page #436
Conversation
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 is fucking awesome and the screenshots look incredible!
Mostly nits and a couple updates
export const PEOPLE_FETCH_FAILED = 'people/fetch/FAILED'; | ||
|
||
export interface PeopleFetchInitiatedPayload { | ||
ids: (Id | Slug)[]; |
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.
nit: in future we can just use a canonicalId type
<span> | ||
{labels.map((label, idx, all) => { | ||
return ( | ||
<React.Fragment key={extractLabel(label)}> |
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 verify in console that you aren't getting Material UI errors here. I used a React Fragment in a MenuItem list and it said React Fragments were usable inside lists, but it may have been Menu specific.
fontSize: '1em', | ||
}} | ||
> | ||
{listLength} | ||
{listLength >= 100 ? '99+' : listLength} |
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 like how this turned out! In the future we could do something where it's a number of "new" items for that smart list in a given period of time, otherwise I imagine most list that are a bit general will always be 99+
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.
most defffff - we'll have to keep track of some counts over time maybe? or maybe we can just estimate based on last view time or how recent items are?
{showPersonFilters | ||
? people && | ||
people.map((person: string) => | ||
personNameBySlugOrId[person] ? ( |
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.
Did you make this for the same issue we had for Popular? If so, should we go the same route and just use 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.
yeah this can probably use ID... i forget why i used slugs lol. there's some other work that needs to be done here so i'll do this in a follow-up
{showResetDefaults ? ( | ||
<Chip | ||
key="Reset_default" | ||
className={classes.networkChip} |
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.
nit: we should think of a more generic class name
onInputChange={debouncedChange} | ||
getOptionLabel={option => nameBySlugOrId[option]} | ||
options={slugz} | ||
filterOptions={opts => opts} |
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.
what is this doing?
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 is some dumb shit in Autocomplete where it tries to filter down the choices based on what your input is... except that our backend is really doing the filtering, so we just want to present the options that are returned from the API without MUI trying to be smart
null, | ||
mapDispatchToProps, | ||
)(ItemCard); | ||
export default connect(null, mapDispatchToProps)(ItemCard); |
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.
lol, we def have different settings somewhere. I noticed my new machine has been formatting like the above and it looks like yours is correcting that. Did you update your line width locally? If so, maybe we update in prettier config?
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.
haha, i dont think i did - we have the prettier config checked into Git dont we? https://github.com/chrisbenincasa/teletracker/blob/master/web/.prettierrc
maybe we just need to define a width in there? is it possible we're using different versions that have different default widths? or maybe my intellij is being a piece of shit?
web/src/containers/Explore.tsx
Outdated
{!loading && items ? ( | ||
<InfiniteScroll | ||
pageStart={0} | ||
loadMore={() => this.loadMoreResults()} |
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.
We can just use this.loadMoreResults
here, otherwise it creates a new callback on every render, this is unavoidable if we are passing stuff into a callback but looks like we aren't 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.
i have no fucking idea how this shit works
web/src/containers/Explore.tsx
Outdated
> | ||
<Grid container spacing={2}> | ||
{items.map((result, index) => { | ||
let thing = thingsBySlug[result]; |
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.
Move to thingsById
to match Popular setup
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.
Fixed
.filter(u => !_.isUndefined(u)) | ||
.map(u => u!) | ||
.value(); | ||
} |
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.
fucking classic christian
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.
lololol
Also: people search / filtering (on Explore page currently)
9aa912e
to
1981937
Compare
1981937
to
dbd9b8f
Compare
Create dynamic lists from Explore page
Create dynamic lists from Explore page
Also: people search / filtering (on Explore page currently)