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

Add support for persistent superchats #79

Merged
merged 47 commits into from
Jun 17, 2022
Merged

Add support for persistent superchats #79

merged 47 commits into from
Jun 17, 2022

Conversation

KentoNishi
Copy link
Member

Implements #49

image
image

Copy link
Member

@r2dev2 r2dev2 left a comment

Choose a reason for hiding this comment

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

The superchat bar is in light theme when yt is in dark theme when "use yt theme" is selected. I just did yarn start:firefox for this. The theme of the superchat bar works fine for manually set themes however. Everything else seems to be fine.

superchat bar in light theme with yt in dark theme

@r2dev2 r2dev2 self-requested a review May 31, 2022 20:28
Copy link
Member

@ChrRubin ChrRubin left a comment

Choose a reason for hiding this comment

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

Some extra stuff aside from the comments:

  • The ticker bar seems to just disappear randomly and never come back? I experienced this during Watame's BDay live on both Firefox and Chromium. Seems to only affect live streams and not VODs. No errors in the console.

  • If we want to use name for the ticker items instead of profile pic like YTC does, we might want to truncate it in case of people with long names taking up too much space like this one:

image

  • Membership items (not talking about gifts) and membership milestone chats aren't being stickied even though they are on normal YTC.

  • Adding a setting to toggle the ticker bar would be nice.

Comment on lines 19 to 24
:global(::-webkit-scrollbar) {
width: 4px;
height: 4px;
}
:global(::-webkit-scrollbar-track) {
* :global(::-webkit-scrollbar-track) {
background: transparent;
}
:global(::-webkit-scrollbar-thumb) {
background: #888;
}
:global(::-webkit-scrollbar-thumb:hover) {
background: #555;
* {
scrollbar-width: thin;
scrollbar-color: #888 transparent;
Copy link
Member

Choose a reason for hiding this comment

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

webkit-scrollbar-track probably isn't needed if thin scrollbars are removed. Also if we're removing thin scrollbars here on Chrome, we should also remove it on Firefox.

Comment on lines 13 to 17
<div slot="actions">
<Button on:click={() => {
$focusedSuperchat = null;
}} color="primary">Close</Button>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

The close button feels unnecessary.

@ChrRubin
Copy link
Member

ChrRubin commented Jun 6, 2022

Ran with Astel's BDay live and now the ticker bar is always there but new superchats aren't added to it lmao

@KentoNishi
Copy link
Member Author

Membership items (not talking about gifts) and membership milestone chats aren't being stickied even though they are on normal YTC.

this is intentional, imo they dont belong in the superchat bar and it just gets in the way of actual messages that need to be stickied so imma leave that as it is

@KentoNishi
Copy link
Member Author

altho ig there are membership joins with messages, we could add that later possibly

Copy link
Member

@ChrRubin ChrRubin left a comment

Choose a reason for hiding this comment

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

Aside from the absolute div, seems good

Comment on lines 331 to 340
<div class="absolute top-0 w-full">
{#if $enableStickySuperchatBar}
<StickyBar />
{/if}
{#if pinned}
<div class="mx-2 mt-2">
<PinnedMessage pinned={pinned} />
</div>
{/if}
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Sticky bar blocks the top most message since its in an absolute div:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

this is the same with pinned messages btw.

@KentoNishi KentoNishi requested a review from ChrRubin June 14, 2022 08:52
@KentoNishi
Copy link
Member Author

@ChrRubin

2022-06-13.22-50-52.mp4

@ChrRubin
Copy link
Member

ChrRubin commented Jun 16, 2022

LMAO I assumed you were gonna just put the sticky bar as a separate div above the "main" HC div like how YTC does it:

image

Could you try to do that instead, or is there some other problem preventing that? Seems to me like it'll involve a lot less fucking around with the heights and stuff and will fix this OCD trigger:

2022-06-16.18-17-40.mp4

Comment on lines 429 to 432
chrome.windows.create({
url: request.url,
type: 'popup',
height: 420,
width: 690
type: 'popup'
}, () => {});
Copy link
Member

Choose a reason for hiding this comment

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

We sure we wanna remove the width/height? The defaults seems a tad large.

image

@ChrRubin
Copy link
Member

I've fixed the OCD trigger in ba7bc8d, so the sticky bar is no longer in an absolute div. Pinned messages not blocking the top few messages is nice so we should still keep that part regardless.

For the popup, I think we should still specify a certain size instead of letting the browser wing it and fill up half of the screen. Maybe slightly larger than before if needed.

@KentoNishi
Copy link
Member Author

@ChrRubin chrome now inconsistently enforces a minimum pop-up window area size based on the percentage of area it occupies. I realized this while deving on my laptop -- chrome throws an error if a window takes less than 50% of available screen area, blocking the pop-up. Getting rid of the dimensions is the easiest solution, unless u wanna try messing with measuring screen dimensions and sht lol

@KentoNishi
Copy link
Member Author

btw @ChrRubin here's the error, screenshotted from the production chrome listing

image

@ChrRubin
Copy link
Member

btw @ChrRubin here's the error, screenshotted from the production chrome listing

image

Jesus that's stupid. No wonder the default literally just fills up half of the screen on Chrome.

Copy link
Member

@ChrRubin ChrRubin left a comment

Choose a reason for hiding this comment

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

@KentoNishi you should double-check that my commit doesn't mess up any of the stuff you changed. Seems good to me at quick glance

@KentoNishi
Copy link
Member Author

Got it, will do after I get back to mah hotel

@KentoNishi KentoNishi merged commit 5537459 into master Jun 17, 2022
@KentoNishi KentoNishi deleted the sticky-superchats branch June 17, 2022 06:58
ChrRubin added a commit that referenced this pull request Jul 5, 2022
Both files are reverted to pre #79
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.

3 participants