Skip to content
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

Stream shown for today can be wrong #96

Open
christophrumpel opened this issue Oct 10, 2021 · 7 comments
Open

Stream shown for today can be wrong #96

christophrumpel opened this issue Oct 10, 2021 · 7 comments
Labels
bug Something isn't working Hacktoberfest

Comments

@christophrumpel
Copy link
Owner

CleanShot 2021-10-10 at 15 12 18@2x

Scheduled start: 2021-10-10 23:00:00 (UTC)

Still stream is shown under today which is wrong because it's tomorrow 1 am.

@christophrumpel christophrumpel added the bug Something isn't working label Oct 10, 2021
@christophrumpel
Copy link
Owner Author

The issue:

CleanShot 2021-10-11 at 15 54 34@2x

@christophrumpel
Copy link
Owner Author

One way to handle this could be to rename "today" and "tomorrow" to "soon", but I like that we have "today" and then only show the time and not the full date anymore.

I guess we have to check the user's IP again and determine the timezone from this?

@christophrumpel christophrumpel changed the title Stream shown for today is wrong Stream shown for can be wrong Oct 14, 2021
@christophrumpel christophrumpel changed the title Stream shown for can be wrong Stream shown for today can be wrong Oct 14, 2021
@christophrumpel
Copy link
Owner Author

If you want to work on this issue, please talk to me about your solution before you do a PR.

@tweichart
Copy link
Contributor

@christophrumpel
when it comes to ordering items based on the user's timezone i always try to avoid pre-ordering on the server side. you don't have the option to send in the browser's timezone as an additional query parameter here (as it's no ajax request), so the only way really i'd say would be to get it via IP. i've never used services for this, given that this needs to query an external service though i'd believe it would not really benefit the page load after all, apart from the fact that it'd probably fail for vpn users etc. also receiving / analysing ip addresses might get quite ugly quite fast if not being careful here (gdpr, log files, etc)

as for the general feature set, "today" as some sort of category seems like a nice addition for the page, i personally would prefer a "today" category so i can easily check what's up today.

that being said, i'd aim for a deferal loading of the stream-list component. for this, i'd change resources/views/livewire/stream-list.blade.php from

<div class="w-full max-w-6xl mx-auto py-24 space-y-16" id="scrollTop">

to e.g.

<div class="w-full max-w-6xl mx-auto py-24 space-y-16" id="scrollTop" wire:init="loadStreams(Intl.DateTimeFormat().resolvedOptions().timeZone)">

this has two effects. first, the stream-list component will not be rendered initially (won't be included in the returned doc request but whole page will be though, hence no waiting for a load of the page). Second, the method that is called on server-side deferred render (loadStreams) will have the timezone as a parameter.

to get the timezone now in the server-side lifewire component class we can add the timezone as public attribute and use it in the ordering as previously in app/Http/Livewire/StreamList.php:

<?php

namespace App\Http\Livewire;

use App\Actions\SortStreamsByDateAction;
use App\Models\Stream;
use Illuminate\Contracts\View\View;
use Livewire\Component;
use Livewire\WithPagination;

class StreamList extends Component
{
    use WithPagination;

    public $timezone = null;

    public function loadStreams($timezone)
    {
        $this->timezone = $timezone;
    }

    public function render(): View
    {
        $streams = Stream::approved()
            ->upcomingOrLive()->fromOldestToLatest()
            ->paginate(10);

        return view('livewire.stream-list', [
            'streamsByDate' => $streams->setCollection(
                (new SortStreamsByDateAction())->handle($streams->getCollection(), $this->timezone)
            ),
        ]);
    }
}

app/Actions/SortStreamsByDateAction.php's handle method now in turn needs to accept a second parameter $timezone, returning the result as before but just checking the timezone as well. if $timezone is null it simply returns a collect() (so the initial render is empty).

i could give this a go if it makes sense ;-)

@christophrumpel
Copy link
Owner Author

That's an interesting idea, I didn't think of yet. What would disadvantages of this approach? Maybe SEO cause Google does not see any streams when checking the site?

@tweichart
Copy link
Contributor

Seo is definitely the one you want to think about here. I'm not a seo expert but afaik vue/react etc are being rendered and indexed properly these days without problems. If I remember correctly, each googlebot has some sort of time it allows the page to render in, everything "being too late" won't be indexed. I think it was somewhere around 300-500ms, but that might be an urban myth as well tbh 😬

In general the doc renders faster as there's no sql query to be made to get the list of streams and also less computing time for ordering the list (although that should be the least problem). What renders slower though is the stream list, as it needs an additional request to the server, run the sql query, render the html output and send it back. The content of the stream list is important though, as it contains the external links to YouTube etc.

i know this doesn't really answer the question if it's good or not, but that's my thoughts on it. In this instance now I'd say it comes down to traffic, server under heavy load might be a little slow when returning the list, if it's constant and googlebot tries to index as well (server cache won't help then) it might fail. This scenario though isn't too likely I'd say, therefore I'd go with the solution mentioned above 🙂

@christophrumpel
Copy link
Owner Author

oh btw thanks for your feedback @tweichart. I just couldn't decide yet which way to go here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Hacktoberfest
Projects
None yet
Development

No branches or pull requests

2 participants