LBRYCommunity/lbry-desktop

Fork 0

Livestream category improvements #7115

Merged

infinite-persistence merged 15 commits from ip/category.livestream into master

2021-09-24 16:26:22 +02:00

infinite-persistence commented

2021-09-17 13:34:29 +02:00

(Migrated from github.com)

Status

Sept 23rd: Pending reviewing on WildWest handling

Code review

I think it would be easiest to go through the commits one-by-one, as the deltas will be more readable, and I've added notes for each of them.
If approved, assigned back to me, and I'll clean up the commits before merging.

Test

https://kp.odysee.com

Objectives

Closes part of #6015
- The eye count blocked by a backend bug.
Closes #7123
Reduce excessive rendering on ClaimTileDiscover

Overview of changes

Reverted the way I added livestreams to the list.
Fetch livestream claims separately via redux, and pass to the list components using the existing prefixUris prop. No more double claim_search calls.

Small differences

Previously, the total number of tiles will not be greater than requested, as the livestream tiles replaces some of the normal tiles. Now, it just appends at the top, so we could see more than requested. Can change back if this is not desired, but I opted for lesser code this round to keep the Claim**Discover components from getting out of hand.

## Status _Sept 23rd_: Pending reviewing on WildWest handling ## Code review - I think it would be easiest to go through the commits one-by-one, as the deltas will be more readable, and I've added notes for each of them. - If approved, assigned back to me, and I'll clean up the commits before merging. ## Test https://kp.odysee.com ## Objectives - Closes part of #6015 - The eye count blocked by a backend bug. - Closes #7123 - Reduce excessive rendering on `ClaimTileDiscover` ## Overview of changes - Reverted the way I added livestreams to the list. - Fetch livestream claims separately via redux, and pass to the list components using the existing `prefixUris` prop. No more double `claim_search` calls. ## Small differences - Previously, the total number of tiles will not be greater than requested, as the livestream tiles replaces some of the normal tiles. Now, it just appends at the top, so we could see more than requested. Can change back if this is not desired, but I opted for lesser code this round to keep the `Claim**Discover` components from getting out of hand.

tzarebczan (Migrated from github.com) reviewed 2021-09-17 13:34:29 +02:00

jessopb (Migrated from github.com) approved these changes 2021-09-17 17:03:55 +02:00

jessopb (Migrated from github.com) left a comment

A thing of beauty.

jessopb commented

2021-09-17 17:04:16 +02:00

(Migrated from github.com)

Is this only for claim_tiles_discover?

tzarebczan commented

2021-09-17 17:35:20 +02:00

(Migrated from github.com)

"Previously, the total number of tiles will not be greater than requested," - the old method worked this way too - the only concern I have for this is how it makes the tiles uneven and this breaks infinite scroll in some cases. Doesn't need to be addressed here, I believe there's a separate issue or we can file one.

Will give this a test soon.

tzarebczan commented

2021-09-17 20:48:17 +02:00

(Migrated from github.com)

This is looking great so far! we'll need to see how to handle the wild west page with the larger number of streams now showing up there since this way is more correct.

I am seeing new live + claim search for live streams when hitting each category - did we want to avoid that? Maybe only call if we haven't refreshed streams in last X minutes? Each time they'll return the same thing in a short period of time.

This is looking great so far! we'll need to see how to handle the wild west page with the larger number of streams now showing up there since this way is more correct. I am seeing new live + claim search for live streams when hitting each category - did we want to avoid that? Maybe only call if we haven't refreshed streams in last X minutes? Each time they'll return the same thing in a short period of time.

infinite-persistence commented

2021-09-18 03:15:34 +02:00

(Migrated from github.com)

Is this only for claim_tiles_discover?

It's for both ClaimTilesDiscover and ClaimListDiscover, if you're referring to the livestream injection.
For render optimization, it's only being done for ClaimTilesDiscover, for now. Keeping ClaimListDiscover simple until he have a cleaner infinite scroll implementation.

> _Is this only for claim_tiles_discover?_ It's for both `ClaimTilesDiscover` and `ClaimListDiscover`, if you're referring to the livestream injection. For render optimization, it's only being done for `ClaimTilesDiscover`, for now. Keeping `ClaimListDiscover` simple until he have a cleaner infinite scroll implementation.

infinite-persistence commented

2021-09-20 04:36:54 +02:00

(Migrated from github.com)

ee41dcf to 4cb83fc

Added a default minimum of 5 minutes between fetches. Clients can bypass this through forceFetch if needed.
Rebased.

### ee41dcf to 4cb83fc - Added a default minimum of 5 minutes between fetches. Clients can bypass this through `forceFetch` if needed. - Rebased.

tzarebczan commented

2021-09-22 17:44:23 +02:00

(Migrated from github.com)

@infinite-persistence I spoke to Julian about the wild west section + lives - we decided it would be good to show a toggle on top to show/hide live streams. Default would be hide, and would be persisted. If show is clicked, then it pops them all on top, followed by non lives.

tzarebczan commented

2021-09-23 02:44:15 +02:00

(Migrated from github.com)

Jeremy actually didn't like the idea of a toggle at the top, defaulted to off.

Maybe instead we'd show a single top line, ordered by trending, and then have a view all livestreams button below it?

Any other ideas on how to show some at first, and then view all?

Jeremy actually didn't like the idea of a toggle at the top, defaulted to off. Maybe instead we'd show a single top line, ordered by trending, and then have a view all livestreams button below it? Any other ideas on how to show some at first, and then view all?

infinite-persistence commented

2021-09-23 11:22:09 +02:00

(Migrated from github.com)

Jeremy actually didn't like the idea of a toggle at the top, defaulted to off.
Maybe instead we'd show a single top line, ordered by trending, and then have a view all livestreams button below it?
Any other ideas on how to show some at first, and then view all?

4cb83fc to e7f840a

WildWest: limit to 8 livestreams initially, and user-expandable to show the rest. Applied to both Tile and List layout.
Rebased to latest master.

> _Jeremy actually didn't like the idea of a toggle at the top, defaulted to off._ > _Maybe instead we'd show a single top line, ordered by trending, and then have a view all livestreams button below it?_ > _Any other ideas on how to show some at first, and then view all?_ ### 4cb83fc to e7f840a - WildWest: limit to 8 livestreams initially, and user-expandable to show the rest. Applied to both Tile and List layout. - Rebased to latest `master`.

tzarebczan commented

2021-09-23 18:39:25 +02:00

(Migrated from github.com)

I like this - but can it respect the screen sizes we have? This looks strange on larger monitors:

I like this - but can it respect the screen sizes we have? This looks strange on larger monitors: ![image](https://user-images.githubusercontent.com/8120721/134548437-14246823-68c4-4867-b6f7-ab2ed7bee80d.png)

infinite-persistence commented

2021-09-24 06:48:55 +02:00

(Migrated from github.com)

but can it respect the screen sizes we have? This looks strange on larger monitors:

e7f840a to `1a8f82b`

Handle tile-count on large screens the same way the homepage does it.
Rebased.

> _but can it respect the screen sizes we have? This looks strange on larger monitors:_ #### e7f840a to 1a8f82b - Handle tile-count on large screens the same way the homepage does it. - Rebased.

tzarebczan commented

2021-09-24 16:26:18 +02:00

(Migrated from github.com)

Thank you, looking great!