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

Issue 1440 hide comments count [ +AND discussing the Project's Issue & Future.... ] #1464

Merged

Conversation

kaplantm
Copy link
Contributor

#1440

Adds option in Appearance -> Comments to hide comments count.
Looks like the issue of the view count hiding comments (as described in #1440) was already addressed.

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

ImprovedTube commented Oct 21, 2022

hi @kaplantm great! hope you didnt have much work with the language files #comments #count
looks good, like permanent css #1440 wasnt addressed

Good day @dodieboy! did you watch "v4.0"? So sorry many of your PRs aren't live for all users yet!
Will try to publish some notes/plan now (none yet). Victor left & we never were late like this.
glad you guys are around

@dodieboy
Copy link
Member

dodieboy commented Oct 21, 2022

@ImprovedTube I just saw the 4.0 update. Many feature is not working for example the playerSize function. I'm working on that feature now.

Sad to hear that Victor left 😭

@ImprovedTube
Copy link
Member

ImprovedTube commented Oct 21, 2022

@dodieboy
we need to sync/merge changes. v4.0 replaced the .js & .css files. Some code wasnt migrated yet.
& it misses all the PRs of the last x Month. - it mainly stopped in progess some month ago.
-Yet v4 prepared:

  • 1: manifest3 (required to make any update starting 2023(Jan1)
    [summary]https://developer.chrome.com/docs/extensions/mv3/mv3-migration/#feature-summary)
    (bad for many extensions. not very much in our case)

  • 2: apparently going back to a more split file-structure because somebody asked (optional)

  • 3: our goal also was to add some efficient features capable to run globally (for every video)
    not sure if thought about this is in the code already?...

Don't know yet, if it will be faster to merge from v4 to v3.965 - or from v3.965 to v4.0. - Just know that all the work is worthy.

@ImprovedTube
Copy link
Member

ImprovedTube commented Oct 21, 2022

v3.965 seems most functional right now.
(October7: https://github.com/code-for-charity/ImprovedTube-for-YouTube/tree/HEAD@{2022-10-07} )

  • So for today we could upload 3.965 to all browser Stores.

Stable v3.945 & Beta 3.1001 in Chrome Webstore were from Victor to test some v4 code at that time

@dodieboy
Copy link
Member

Yup, 3.965 look good to me (still have some bugs but is better than 3.945). I'm still porting all my past work to V4 now. Hope that I can finish porting everything before I go.

@ImprovedTube
Copy link
Member

ImprovedTube commented Oct 21, 2022

Yay!! 🥳

Yup

unless there are any never commited fixed or necessary changes for manifest3
So while we need to watch all new code, there are 3 To-Do's, (& the question comes to mind if/how we should to split the code to be easier)

#1439

@ImprovedTube
Copy link
Member

@dodieboy we need to sync/merge changes v4.0

( in short, it comes undocumented so we should notice all gaps(missing code)
& understand the intented architectural change. thanks again!! )

@ImprovedTube ImprovedTube changed the title Issue 1440 hide comments count Issue 1440 hide comments count [ +AND discussing the Project's Issue & Future.... ] Oct 23, 2022
@ImprovedTube
Copy link
Member

ImprovedTube commented Oct 24, 2022

#1469 🥳🥳🥳 @dodieboy

Besides that some settings still need to be imported then (or names kept) for example you renamed [it-hide-like-button=true] to [it-likes=hidden] #1411 )

Same for #1434. The settings need to either: 1. keep the same names - even if their locales changes, including english - or 2. we need to import the old settings (=We shouldn't make people lose some settings at an update...)

@dodieboy
Copy link
Member

I did not rename it-likes, it was you who break the hide like and dislike button into 2 hide like and hide dislike and to make the code cleaner, i split the hide button and hide text to 2 option.

@dodieboy
Copy link
Member

@ImprovedTube just wondering for #1442, should we make it auto active when "show time remaining" and "player speed" is activated?

@ImprovedTube
Copy link
Member

ImprovedTube commented Oct 29, 2022

I did not rename it-likes, it was you who break the hide like and dislike button into 2 hide like and hide dislike and to make the code cleaner, i split the hide button and hide text to 2 option.

ohh @dodieboy didnt know you saw/ported my commit too :) it was renamed by #1411
your current structure is good! Yet the old dropdown [it-likes=hidden] [it-likes=icons_only] could also be moved on top of your new list "hide detail buttons" for now (unless the values are imported somehow). I added "hide dislikes text" when i made sense to hide this text alone, dislikes was text, while likes ws the only one different (iot a text but a counter)
so generally:

  • local storage ( settings) should still be considered, if renamed it can be imported at the update, (we used to do that several times.)
  • feature doing multiple things can appear on top of details // details con appear conditionally
    • two buttons with interesecting functionality arent wrong either

just wondering for #1442, should we make it auto active when "show time remaining" and "player speed" is activated?

Yes i think so (or always with "time remaining" and call it "dont divide by playback speed" ? And only show that conditionally like our our speede&volume slider)

@dodieboy
Copy link
Member

I have updated my "hide detail buttons" to used the dropdown.

@dodieboy
Copy link
Member

dodieboy commented Oct 29, 2022

Yay!! 🥳

Yup

unless there are any never commited fixed or necessary changes for manifest3 So while we need to watch all new code, there are 3 To-Do's, (& the question comes to mind if/how we should to split the code to be easier)

#1439

I have did some test,

  1. For Background.js error, it is cause by chrome.tabs.sendMessage(). But I not very clear with chrome api, tried to fixed it but failed on my side. Anyone have better knowledge pls help.
  2. improvedtubeYoutubeIcon() is not missing, you can find it in content-script/website-context/youtube-features/setting.js

@ImprovedTube
Copy link
Member

ImprovedTube commented Nov 8, 2022

improvedtubeYoutubeIcon

just our UI. Thanks! @dodieboy
Updated the list above

@ImprovedTube
Copy link
Member

"Anyone, who has better knowledge, pls help"

Noone yet afaik. (Besides generally we could invite the maintainers of another extension to have a look. like Github Refined (possibly mutually interesting.) ( Social Fixer & Simplify )

chrome.tabs.sendMessage

Solution? : https://groups.google.com/a/chromium.org/g/chromium-extensions/c/st_Nh7j3908/m/1muOgSX5AwAJ
( +general documentation😆 )

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