search

lichess-org / mobile

Lichess mobile app v2
GNU General Public License v3.0
1.1k stars 150 forks source link

Broadcast feature, first iteration #726

Closed julien4215 closed 1 week ago

julien4215 commented 1 month ago

broadcast_dark.webm

veloce commented 1 month ago

Looks very good! Thank you. I think we should see the big images even in watch tab root, without having to tap to see more.

What do you plan to add on top of this? I think we should limit the set of features for this PR, that'll be easier to review.

I can review the current state (feature wise) and the rest would be done in other PRs.

julien4215 commented 1 month ago

I'd like to use the radio tower icon as on the website but I didn't find it in the Lichess icons of the mobile app and I don't know how to add it

veloce commented 1 month ago

The radio tower in the website correspond to the study feature, and broadcast are actually implemented as a study. So I'm not sure but this may be the reason why there is that icon in the website.

We won't use it for broadcast in the app, so don't bother with it.

I think this PR should not evolve anymore than just the list of broadcasts and the list of boards for now. It's already big enough to review.

The feature will not be ready yet to be released but we can merge the code when it's ready and hide it, so you can continue to work in another PR when this one is done.

julien4215 commented 1 month ago

I wanted to use the radio tower to replicate this default image for broadcast like here image I thought it would make sense to keep the same default image (a linear gradient with the radio tower icon).

julien4215 commented 1 month ago

Looks very good! Thank you. I think we should see the big images even in watch tab root, without having to tap to see more.

What do you plan to add on top of this? I think we should limit the set of features for this PR, that'll be easier to review.

I can review the current state (feature wise) and the rest would be done in other PRs.

I prefer to keep the list section because it is more coherent with the streamer and live games parts but I can change it if you think it would be better with the big images.

I still don't really know how to proceed after this. I think the biggest challenge is to find an easy way to navigate between all the pages. I think there will be at least 3 pages for a tournament, maybe more (including the grid board which is already started).

Also. in the grid board the positions are not updated, there is only one call which is made when the page is opened where I get the current fen for each game of the tournament. Do you want this to be implemented before reviewing the PR or should it be made in a different one ?

veloce commented 1 month ago

It can be done in a different one imo. I'll give you the radar icon so you can make the image tomorrow.

ijm8710 commented 1 month ago

image

I still don't really know how to proceed after this. I think the biggest challenge is to find an easy way to navigate between all the pages. I think there will be at least 3 pages for a tournament, maybe more (including the grid board which is already started).

My initial suggestion is that once a broadcast is open, it'll have horizontally slidable panels similar to the referenced image (but can also be tapped)

Panels eventually being:

The leaderboard tab would be the tab that (eventually in the future) could have clickable player links that redirect to a "round by round vertical list" for said player's performance throughout that tournament

Unlike the current broadcast design on web, I think the round filter would then be below this 3-tab setup rather than above it

Understood that this design would slightly deviate from the rest of the app but I think regardless that's going to have to happen

julien4215 commented 1 month ago

I think this PR should be ready for a review soon.

I still need to :

Should I add an hidden option to activate the broadcast feature like you activate the developer mode on Android by tapping several times the build number ?

veloce commented 1 month ago

Nice!

Should I add an hidden option to activate the broadcast feature like you activate the developer mode on Android by tapping several times the build number ?

No need to make sth that advanced. But an option that hide the feature by default and you can activate using a --dart-define would be nice.

julien4215 commented 1 month ago

Nice!

Should I add an hidden option to activate the broadcast feature like you activate the developer mode on Android by tapping several times the build number ?

No need to make sth that advanced. But an option that hide the feature by default and you can activate using a --dart-define would be nice.

The idea was to allow also the users that follow closely the app to activate the broadcast feature if they want to

ZTL-UwU commented 1 month ago

Shouldn't it be on by default?

veloce commented 4 weeks ago

Thank you for putting this together! I pushed a new commit with the radio tower asset to main, you can now rebase your branch if you want to use it.

julien4215 commented 4 weeks ago

Thank you for putting this together! I pushed a new commit with the radio tower asset to main, you can now rebase your branch if you want to use it.

Thanks for the icon.

What should we do about the visibility of this incomplete broadcast feature ? I would be in favor of hiding it by default with an hidden option to show this feature for users that follow closely the app.

Shouldn't it be on by default?

@ZTL-UwU I think most users will be confused because the feature is still a work in progress.

veloce commented 1 week ago

Thanks again for this work @julien4215 ! Code is LGTM. Now I'm gonna test it, can you please update your branch (rebase to last main commit)?

julien4215 commented 1 week ago

Right now the sorting is still done on the client side. Ornicar created a new endpoint that includes the sorting https://lichess.org/api/broadcast/top but it also quite different from the previous endpoint as it adds pagination and group for tournaments.

veloce commented 1 week ago

I think we're gonna want to use this new endpoint and add pagination support.

Is it a lot of work for you to change that? Do you prefer me to test this PR as is and do the change in another one?

julien4215 commented 1 week ago

I would prefer to do the change in a different PR because it will require me a lot work to use the new endpoint.

veloce commented 1 week ago

Couple of notes from my live test:

Here we're seeing several "Asian young championship" entries, where we should only see one.

If you tap on a broadcast, it brings you to the games screen, but the title is wrong:

Capture d’écran 2024-06-19 à 10 43 33

Here it shows "Diffusions", we want to see the current broadcast title instead.

Of all the points listed here we should fix in this PR:

@julien4215 ^

julien4215 commented 1 week ago

The grouping issue requires the new endpoint https://lichess.org/api/broadcast/top that ornicar created. And same for live, past and upcoming tournaments.

veloce commented 1 week ago

Let's fix the title issue and I'm merging it then.