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

Fix #2069: Transparent top bar stops working when scroll. #2100

Conversation

joaolscosta
Copy link
Contributor

This commit resolves issue #2069, where the transparent top bar would lose its transparency effect upon scrolling. The issue was addressed by removing unnecessary icons and adjusting opacity settings to maintain the desired transparent appearance even during scrolling.

@ImprovedTube
Copy link
Member

ImprovedTube commented Mar 15, 2024

hi, wow!! a lot of work.

  • new lines: there are some duplicated lines
  • removed lines: please don't remove the features, which can co-exist.
  • opacity 0 3s more accurate opacity 0.25 , 2. seconds or so.

@joaolscosta
Copy link
Contributor Author

Hi by the end of the day i'll have everything fixed. I didn't remove any code from anyone, i just didn't have the most recent commit when i requested. It was my bad but i'll resolve it!
I will make the changes to the code that you recommended too.

This commit resolves issue code-charity#2069, where the transparent top bar would lose its transparency effect upon scrolling. The issue was addressed by removing unnecessary icons and adjusting opacity settings to maintain the desired transparent appearance even during scrolling.
@joaolscosta joaolscosta force-pushed the Transparent-top-bar-stops-working-when-scroll-bug-fix-2069 branch from 4ca11a3 to 1ce6ac4 Compare March 18, 2024 10:07
@ImprovedTube
Copy link
Member

hi @joaolscosta!

  • The JS needs to be wrapped in an if checking our storage if the transparency feature is on
    (else the JS will run for every user)

  • The it-hide-voice-search-button='true' ... needs to stay as is
    (as long as the button stays in the bottom of our header features too)

  • i meant opacity: 0 opacity: 0.25?

@ImprovedTube ImprovedTube marked this pull request as draft March 18, 2024 11:43
@joaolscosta
Copy link
Contributor Author

Hi thanks for the response!

I did't quite understand what it is suposed about the JS file. Can you give me more details? I'm sorry.

For the voice search button you want me to leave it always shown? Is that right? The feature to hide when scroll it is to eliminate?

At the time I understood that it would be to change the opacity from 0 to 0.25. I understood it wrong? What do you mean about "opacity 0 3s more accurate opacity 0.25 , 2. seconds or so.". I'm sorry for the miss understandings.

@ImprovedTube
Copy link
Member

if (ImprovedTube.storage.header_transparent === true) { JS for this feature (maybe none? #2069 }

if (ImprovedTube.storage.header_nameYourNewFeature === true) { JS for the new feature }
( a new button can be added in our menu/skeleton/appearance.js )


sorry, i meant to ask: "why opacity:0 and not a bit more?"


this line (= different feature) need not be removed:

html[it-hide-voice-search-button='true'] #voice-search-button  {
display: none !important;

This commit resolves issue code-charity#2069, where the transparent top bar would lose its transparency effect upon scrolling. The issue was addressed by removing unnecessary icons and adjusting opacity settings to maintain the desired transparent appearance even during scrolling. It also have modifications that admin requested to.
@joaolscosta joaolscosta marked this pull request as ready for review March 18, 2024 19:33
@joaolscosta
Copy link
Contributor Author

Hi @ImprovedTube i just did the suggestions you made it! Hope you like it.

@ImprovedTube
Copy link
Member

hi! @joaolscosta thanks for caring!

if (ImprovedTube.storage.header_transparent === true) { JS for this feature (maybe none? #2069 }
if (ImprovedTube.storage.header_name_your_new_feature === true) { JS for the new feature }
( a new button can be added in our menu/skeleton/appearance.js )

no event listeners need to be added without a (this) toggle being enabled

we can wrap all JS in one IF. (until one day we might use something for or a 2nd feature like your getScrollDirection() )


this line (= different feature) need not be removed:

html[it-hide-voice-search-button='true'] #voice-search-button  {
display: none !important;

💭

@joaolscosta
Copy link
Contributor Author

@ImprovedTube First of all, I am realy sorry for my bad understading of what you are trying to recommend to me.

So i added that if condition inside the functions but you what all of the transparent bar functions inside of that one if, is that right?

And for the line you mention it doesn't need to be removed i just read to remove it ahahah my bad. The feature to disable voice search button while scrolling can be implemented or not?

Thanks for the pacience.

@joaolscosta
Copy link
Contributor Author

@ImprovedTube I just realized what are you saying about the line to be not removed :( .

This commit resolves issue code-charity#2069, where the transparent top bar would lose its transparency effect upon scrolling. The issue was addressed by removing unnecessary icons and adjusting opacity settings to maintain the desired transparent appearance even during scrolling. It also have modifications that admin requested to.
@joaolscosta
Copy link
Contributor Author

@ImprovedTube I just did the modifications you told me to do. Evaluates the changes I made and checks whether the pull request can be accepted.

@ImprovedTube
Copy link
Member

sorry ... understading of what you are trying to recommend to me.

No need to be sorry. Still efficient for me. And i tend to make sentences too long.

  • And collaboration on github is still so rare in the world. And more special, while most people who arrived here, only learned English as teenagers or later.

If you like, i think it will be worth it & motivating if want to note-down/publish your clues for new contributors about this repo. As we don't have much documentation at all, (but extensions are still neglected and getting more popular and we can be more relevant, a framework for extensions developers)

@ImprovedTube ImprovedTube requested a review from raszpl March 20, 2024 07:40
@ImprovedTube
Copy link
Member

ImprovedTube commented Mar 20, 2024

( #2077 #2091 #2097 )

  • Deciding on the naming of options. Does # 2100 introduce a new option? ( (i guess one we can reasonably push changes to existing option, if they fit and are nice or if the option is for fun too. - If less than 50% of them will switch back to the "legacy" version )

should be another separate option, as is it screws with Appearance >Header

EDIT: how about hidden until hover over?

(by @raszpl, #2069)

@ImprovedTube ImprovedTube removed the request for review from raszpl March 20, 2024 07:56
@joaolscosta
Copy link
Contributor Author

@ImprovedTube Suppose I make any changes? I ended up not understanding

@joaolscosta
Copy link
Contributor Author

sorry ... understading of what you are trying to recommend to me.

No need to be sorry. Still efficient for me. And i tend to make sentences too long.

* And collaboration on github is still so rare in the world. And more special, while most people who arrived here, only learned English as teenagers or later.

If you like, i think it will be worth it & motivating if want to note-down/publish your clues for new contributors about this repo. As we don't have much documentation at all, (but extensions are still neglected and getting more popular and we can be more relevant, a framework for extensions developers)

Realy thanks for the words.

You are asking me to comment the code to be more clear what it does?

Thanks @ImprovedTube

@joaolscosta
Copy link
Contributor Author

joaolscosta commented Mar 22, 2024

( #2077 #2091 #2097 )

* [ ]  Deciding on the naming of options.  Does # 2100 introduce a new option? (  (i guess one we can reasonably push changes to existing option, if they fit and are nice or if the option is for fun too. -  If less than 50%  of them will switch back to the  "legacy" version  )

should be another separate option, as is it screws with Appearance >Header

EDIT: how about hidden until hover over?

(by @raszpl, #2069)

So instead of using the already created TransparentColor i create a new line for what i did? Is that it?

@ImprovedTube

@ImprovedTube
Copy link
Member

hi @joaolscosta! maybe. this was just a note, waiting-status. in other words:

  • i didnt test @raszpl's commits & suggestions about header:transparent yet, nor your work.
  • before publishing this, we will decide, if there will be only one "header:transparent" or another sub-option "header:transparent:advanced(....)" - and if this sub-option will be enabled by default or not.

@joaolscosta
Copy link
Contributor Author

hi @joaolscosta! maybe. this was just a note, waiting-status. in other words:

* i didnt test @raszpl's commits & suggestions about `header:transparent` yet,  nor your work.

* before publishing this, we will decide, if there will be only one  "header:transparent"  or another sub-option "header:transparent:advanced(....)" - and if this sub-option will be enabled by default or not.

Oh fine, i read your comments a thousand times and it was what I thought but i needed to clear it.
Thank you! For me I think it can be two different options in header field.

@joaolscosta
Copy link
Contributor Author

@ImprovedTube for the issue you have mentioned my last solution did it work? The issue is still open but with tag Completetion/Revision. Do I need to improve or change something?

Thanks.

@ImprovedTube
Copy link
Member

hi @joaolscosta! it just seemed @Anoaxx might like to try already. @Anoaxx often contributes ideas.
as members of the team all of us can close or label threads. inviting you

This commit resolves issue code-charity#2069, where the transparent top bar would lose its transparency effect upon scrolling. The issue was addressed by removing unnecessary icons and adjusting opacity settings to maintain the desired transparent appearance even during scrolling. It also have modifications that admin requested to. Removed the filter top bar of the main page.
@joaolscosta
Copy link
Contributor Author

Hi @Anoaxx !! i just removed the filter bar you have mentioned.

@ImprovedTube I followed your recomendation of inspecting the page and I fixed it pretty quickly. I opted to remove it. Thanks.

@joaolscosta
Copy link
Contributor Author

I was thinking and instead of removing it at all should I remove the chips bar just when we scroll? @ImprovedTube @Anoaxx

@joaolscosta
Copy link
Contributor Author

Hi! Is there anything i can do more? @ImprovedTube

@ImprovedTube
Copy link
Member

hi! @joaolscosta thanks! and sorry i missed your new work! Generally, since we already have a whole settings page for the header alone (appearance:header), e can keep working with that. ( Hide voice search button. Or, when we change something, consider what settings people used before or not (legacy) ) And we can still consider more stuff @raszpl said in #2069 ( Appearance:Header:Icons:Faint until hover ? Such toggles or options (if we get any more) can enable/disable each other like this: (what we do else where:)
auto pause
(of course in all cases this is just one way to do it/and make users understand)

...So now equally for this "chips"-bar, we could also add a toggle and a drop-down

Header [hidden; hover]
Tags (above feeds): [move to the top when the header isn't visible; hide; hover}

Header: Transparent
Tags (above feeds): transparent;


So this can be a UI-/UX-challenge
(sorry😅)



Meanwhile i might wonder about things like Youtube's new layout experiment. (Comments on the side)
#2178 (unless i might hope for a Pull request for this, since little is missing, and there is some other work to do too (manifest3), etc)

@joaolscosta
Copy link
Contributor Author

Hello @ImprovedTube ! I fully understand what you said, but we also have to understand that the bug that caused this PR is resolved and what you are asking me for are new bugs/features that have to be resolved.

I am available to resolve new bugs in a new PR but this must already be completed (considering my skills I will help).

I also understand the impact of user reviews but the functionality I am correcting was already there and did not work, with my changes the functionality is implemented and I am contributing to making the extension usable.

If we implement things that one day will stop working we would never implement anything since it is not an official YouTube extension, we have to adapt as they change the UI.

@joaolscosta
Copy link
Contributor Author

Just a check if you missed the message again. It's fine!!! @ImprovedTube

@ImprovedTube
Copy link
Member

ImprovedTube commented Apr 19, 2024

hi! @joaolscosta,
didn't ask, just shared.
Of course, often one line is enough for a PR, if you like.


if (window.scrollY === 0) { if (window.scrollY > 1 + window.innerHeight * 0.001; ) {


btw (if you want), a scrolling eventlistener can be a in our functions.js / used more often. ( #2149 (comment) )

joaolscosta and others added 2 commits April 20, 2024 18:33
This commit resolves issue code-charity#2069, where the transparent top bar would lose its transparency effect upon scrolling. The issue was addressed by removing unnecessary icons and adjusting opacity settings to maintain the desired transparent appearance even during scrolling. It also have modifications that admin requested to. Removed not necessary lines.
@joaolscosta
Copy link
Contributor Author

Hi again @ImprovedTube !! Thanks for your support!! About the event listenner appreciate for your comment because i used that for something i did before but don't realy need it anymore. I understood which is the right way to do the scroll event but i just removed the lines and commited.

@joaolscosta
Copy link
Contributor Author

@ImprovedTube

@ImprovedTube ImprovedTube force-pushed the master branch 2 times, most recently from 62a36d2 to 40aedd4 Compare April 27, 2024 17:02
@joaolscosta
Copy link
Contributor Author

@ImprovedTube hey admin!! :)) Is there anything else i can contribute to here?

@ImprovedTube
Copy link
Member

ImprovedTube commented May 5, 2024

hi! :) @joaolscosta. Thanks for caring! You choose!
I might be typing "too much" already (like #1445).
And 😅 should be busy here to test/fix our Manifest3 version, which will be enforced starting June1.

@ImprovedTube
Copy link
Member

(github doesnt notice it is only white space changes)

some of your CSS is overwriting each other, the first version has no effect:

/* Change opacity when scrolling down */
html[it-header-transparent=true][data-scroll-direction=down] ytd-masthead #end {
	opacity: 0;	
}
/* Make other icons barely visible */
html[it-header-transparent=true][data-scroll-direction=down] ytd-masthead #end {
	opacity: 0.7 !important;
}

you can review your CSS

and we can add the button called something like transparent(alternative) or transparent(advanced)(when scrolling down) in our menu/...appearance.js header section.

@ImprovedTube ImprovedTube marked this pull request as draft May 8, 2024 00:39
This commit resolves issue code-charity#2069, where the transparent top bar would lose its transparency effect upon scrolling. The issue was addressed by removing unnecessary icons and adjusting opacity settings to maintain the desired transparent appearance even during scrolling. It also have modifications that admin requested to. Added an alternative choice in apperance to this transparent header.
@joaolscosta
Copy link
Contributor Author

Hi @ImprovedTube!!
Thanks for your advices! I just commited all the changes you asked for.
I think it was a good idea to have both of the transparent header variants.
I removed not necessary lines.
Thanks and waiting for the review!!!

@joaolscosta joaolscosta marked this pull request as ready for review May 11, 2024 10:49
@ImprovedTube ImprovedTube merged commit 8239a2d into code-charity:master May 12, 2024
@ImprovedTube
Copy link
Member

hi @joaolscosta, thank you for caring & patience!

..I don't know. Nothing has to be final. Maybe the word scroll should appear in the name or a nice/promising word like 'advanced'? Or would it be good to be one combined feature, with one or two sub-options, appearing below it once activated?

https://github.com/code-charity/youtube/releases/tag/v4.920rev5

@joaolscosta joaolscosta deleted the Transparent-top-bar-stops-working-when-scroll-bug-fix-2069 branch May 15, 2024 15:15
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