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

Porting of old commit #1469

Merged
merged 33 commits into from
Oct 29, 2022
Merged

Porting of old commit #1469

merged 33 commits into from
Oct 29, 2022

Conversation

dodieboy
Copy link
Member

@dodieboy dodieboy commented Oct 23, 2022

Finally port and tested all the old commit. Took awhile to figure out the new format.

Close #1379
Close #1369

@ImprovedTube
Copy link
Member

ImprovedTube commented Oct 23, 2022

Woaw! 😍

figuring out

  • Then does the new format make sense to you regarding?

3. Code is split into several files


Also: #1464 (comment)

@dodieboy
Copy link
Member Author

dodieboy commented Oct 24, 2022

Yup, I'm just too use to the old format, that's why the hide control button previously is not work (I place the code at the wrong location 🙄🤦)

@ImprovedTube
Copy link
Member

ImprovedTube commented Oct 24, 2022

hi! :) normal! serious question, i meant we are free to chose the exact future format😅 💭💭
lucky to have you!

Copy link
Member

@ImprovedTube ImprovedTube left a comment

Choose a reason for hiding this comment

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

how come playerSize() & init.js

@dodieboy
Copy link
Member Author

dodieboy commented Oct 26, 2022

I did not see any playerSize() function so I removed it. I believe player size now use CSS to do the change.

I also remove some doubled run code to optimize the code.

@dodieboy
Copy link
Member Author

lol, I spell streaming as steaming

Copy link
Contributor

@hboyd2003 hboyd2003 left a comment

Choose a reason for hiding this comment

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

Looks good!

Fixed "ImprovedTube.playerAutopauseWhenSwitchingTabs is not a function" error
@dodieboy
Copy link
Member Author

@ImprovedTube I think can merge this PR alr.

it will only show when show-remaining-duration is enabled
@ImprovedTube
Copy link
Member

ImprovedTube commented Oct 29, 2022

I also remove some doubled run code to optimize the code.

yes! thank you! & sorry to answer late!
and init.js? 402d705#r88268616

As said, since the changes came undocumented and unfinished, we can only guess.

playerSize is maybe the most used feature & had 100+ lines. Maybe the reference remained, unused to still add a new version? I had wondered long ago if it cant be CSS only, but i dont know if thats a reason now.
( We can test CSS only, or read the old playerSize function still https://github.com/code-for-charity/ImprovedTube-for-YouTube/blob/28f1bef13c0f77c2b9c5200ec9c7868b8d354629/youtube-scripts.js#L1140 ( shorter in 3.1001: https://crxcavator.io/source/lodjfjlkodalimdjgncejhkadjhacgki/3.1001?file=content-scripts%2Fcontent-scripts.js&platform=Chrome
)


(And/more strikingly gotta review background.js, satus.js #1464 (comment) )

@dodieboy
Copy link
Member Author

Nice find, it look like it is for custom player size. I updated the old playerSize() so that it depend more on CSS and not JS as JS code is create lag.

@ImprovedTube ImprovedTube merged commit d884577 into code-charity:master Oct 29, 2022
@ImprovedTube
Copy link
Member

ImprovedTube commented Oct 29, 2022

🥰

init.js? 402d705#r88268616
As said, since the changes came undocumented and unfinished, we can only guess.

*so obviously, while some lines can always be shortened, others might have evoled over several steps/bug reports/years, so that not every reason will be guessed, so it would be handy to always see the whole history per line. And if we have to think about one all over, then we could document all our conclusion / reasons.

(merging without much review to raise tension 😆
Hopefully sending an update to many/most users soon.)

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.

Display Upload Day-Of-Week Change for Hover Search Bar
3 participants