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

Initial commit #1

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Initial commit #1

wants to merge 29 commits into from

Conversation

dubeyShivank
Copy link
Collaborator

No description provided.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/lib/Timeline.svelte Outdated Show resolved Hide resolved
src/pico.css Outdated Show resolved Hide resolved
src/lib/model.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
Copy link

@cTopher cTopher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need a way of deploying this to deploy this. Based on https://bitbucket.org/r8szq0vy/theolive-player/src/master/web/demo/scripts/deploy.ts you could make a github action.

@dubeyShivank
Copy link
Collaborator Author

We also need a way of deploying this to deploy this. Based on https://bitbucket.org/r8szq0vy/theolive-player/src/master/web/demo/scripts/deploy.ts you could make a github action.

Do we want to deploy from here or from our internal repo? I removed the steps and the deploy scripts assuming we'd want to keep deployments from the internal repo but I'm ok to do it either way.

@cTopher
Copy link

cTopher commented Jan 9, 2025

Do we want to deploy from here or from our internal repo? I removed the steps and the deploy scripts assuming we'd want to keep deployments from the internal repo but I'm ok to do it either way.

I would do it here and fully remove all demo code from the old repo once this is merged. Make sure that all data that should not be publicly visible to be inside secrets.

package.json Outdated Show resolved Hide resolved
src/routes/+page.svelte Outdated Show resolved Hide resolved
src/routes/+page.svelte Outdated Show resolved Hide resolved
src/lib/analytics.ts Outdated Show resolved Hide resolved
src/lib/Channel.ts Outdated Show resolved Hide resolved
src/lib/Stats.svelte Outdated Show resolved Hide resolved
src/lib/Stats.svelte Outdated Show resolved Hide resolved
@cTopher
Copy link

cTopher commented Jan 9, 2025

image

For the chart I would:

  • increase the size on big screens
  • hide the legend
  • reduce the amount of x axis labels
  • set min y value to 0 (there should not negative values)
  • remove opacity

src/lib/Player.svelte Outdated Show resolved Hide resolved
@dubeyShivank
Copy link
Collaborator Author

dubeyShivank commented Jan 10, 2025

image

For the chart I would:

  • increase the size on big screens
  • hide the legend
  • reduce the amount of x axis labels
  • set min y value to 0 (there should not negative values)
  • remove opacity

How do you find this? I tweaked the alpha so we don't obstruct the content behind the timeline but can make changes if you suggest :)
image
image

On a side note - on small screens if the demo has been running for a while, the graph is unreadable. I suggest we should have a max of the last 20s(?) worth of data points. What do you think? I'll keep the current one minute for bigger screens

@dubeyShivank
Copy link
Collaborator Author

Do we want to deploy from here or from our internal repo? I removed the steps and the deploy scripts assuming we'd want to keep deployments from the internal repo but I'm ok to do it either way.

I would do it here and fully remove all demo code from the old repo once this is merged. Make sure that all data that should not be publicly visible to be inside secrets.

Easy, I've added a basic deployment script now, which will deploy on merge to main. But we can play around with the specific rules on when to deploy and what our approach to auto-updating version numbers is.

onMount(() => {
if (!browser) return
const searchParams = new URLSearchParams(window.location.search)
channelId = searchParams.get('channel') ?? undefined
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not updated when the goto triggers. (you can reproduce this by navigating to http://localhost:5173, it will always have channelId undefined)

background: black;
margin: 0;
padding: 0;
height: 100%;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a duplicate


<style>
.container {
background-color: rgba(255, 255, 255, 0.15);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the video is white, all the labels become invisible (you can fix this by using background-color: rgba(0, 0, 0, 0.5);)

border-radius: 4px;
z-index: 1001;
left: 0 !important;
background-color: rgba(255, 255, 255, 0.1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one also becomes invisible on a white video, you could change it to background-color: rgba(0, 0, 0, 0.3);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants