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

Update search.js polish, bugs fixed, remember/restore last search query and position. #2201

Merged
merged 23 commits into from
Apr 27, 2024

Conversation

raszpl
Copy link
Contributor

@raszpl raszpl commented Apr 23, 2024

Update search.js keydown is redundant, input handles same case

Update search.js clean variable mess

Update search.js text_field was always broken

Update satus.js removing redundant addEventListener
All the listeners are already setup by satus.render. code here was doubling them down resulting in Search being performed twice every time.

Update satus.js respect storage: false on text-field elements
Seatch text-field (Input) is of type component.skeleton.storage: false

Update satus.js propagate manual change 'text-field' value
trigger 'change' event same way as in element.storage.set

parent.dispatchEvent(new CustomEvent('change'));

Update satus.js more graceful handling of clicking outside of modal dialog
no more double .close()

Update search.js phantom style set
we never set style on text-field to begin with, no reason to try and remove it either

Update satus.js stop adding storage component to storage: false elements

Update satus.js make ok() cancel() optional for modal.confirm simplified variant

Update search.js remember last search query and search_results scroll position, restore on search reopen

Update skeleton.js search restore variables

funnily enough self is somehow still set while in this handler and points to same element as text_field
All the listeners are already setup by satus.render. code here was doubling them down resulting in Search being performed twice every time.
Seatch text-field (Input) is of type component.skeleton.storage
@ImprovedTube ImprovedTube marked this pull request as ready for review April 24, 2024 02:06
@ImprovedTube ImprovedTube marked this pull request as draft April 24, 2024 02:13
@ImprovedTube
Copy link
Member

on a quick/superficial glance:
does satus.js have the same logic? https://github.com/search?q=repo%3Acode-charity%2Fyoutube%20backspace&type=code

(much of all code was originally written for this extension first, then moved to satus)

text-field search.on.click?

@raszpl
Copy link
Contributor Author

raszpl commented Apr 24, 2024

on a quick/superficial glance: does satus.js have the same logic? https://github.com/search?q=repo%3Acode-charity%2Fyoutube%20backspace&type=code

you linked to the search.js keydown bit I am deleting :)

text-field search.on.click?

thats the convention used by search.js

@raszpl raszpl changed the title Update search.js keydown is redundant, input handles same case Update search.js polish, bugs fixed, remember/restore last search query and position. Apr 24, 2024
@raszpl
Copy link
Contributor Author

raszpl commented Apr 26, 2024

fdf3d80 returns categories(buttons) in search. There are limitations - only one element of the same name can be returned. Problem elements are:
player_hide_controls_options. #2210 made it a redirect so both spots in Player & Appearance->Player render same element defined in Appearance->Player. Not a problem.
below_player_screenshot
below_player_pip
below_player_loop, those three are [Extra buttons below the player] in two places, Player & Appearance->Details->Buttons (below the player). I havent thought about ways of making it render from same spot yet. Even without redirect its not a problem, simply one from Appearance->Details->Buttons (below the player) will be returned
comments - this is a slight problem. Apperance.js defines two elements with same name:

extension.skeleton.main.layers.section.appearance.on.click.comments - category(button) opening whole card full of options

extension.skeleton.main.layers.section.appearance.on.click.comments.on.click.comments - one of those options

need to rename one of them

@raszpl
Copy link
Contributor Author

raszpl commented Apr 26, 2024

aaand thats it.

  • search "player"
  • scroll down to Appearance
  • click on Player

Search results disappear, Player Category opens.

  • click on loupe icon

Search results appear again in same spot it was when being hidden

  • click Shortcut/Popup player

Shortcut popup appears, Search results are still underneath

  • click cancel or anywhere outside of popup to close it
  • click on hamburger menu
  • click settings

again works as expected. Etc etc :-)

Few things to look in the future is:

1 remembering Search in History, so instead of clicking Loupe to get back to results user could click "<" back icon and get History.

2 adding ^ icon next to <. < goes Back, ^ would go Up in hierarchy.

3 Clicking on category breadcrumbs (Appearance > Player) in search results automagically redirecting to appropriate pages, enabling:

  • search for Player
  • click on "Appearance" of Appearance > Player
  • land in Appearance

4 When Extension popup first appears Search is activated with blinking cursor. This is great for discovery! But as soon as user clicks on one of Categories (for example Player) Search should close so Header shows Section name and < back icon. Currently going back is super annoying with having to click two times, first to close search we didnt use, then < back.

@raszpl raszpl marked this pull request as ready for review April 26, 2024 16:28
@raszpl raszpl mentioned this pull request Apr 26, 2024
@ImprovedTube
Copy link
Member

Apperance.js defines two elements with same name
...
need to rename one of them

maybe it should be possible? (if satus.js is a framework)
yet semantically it points to potential improvements: appearance elements (comments, sidebar, header) have the option to hide them completely. So for the most precise UX hiding any of them completely should would grey out the element on the appearance screen and lock all the suboptions. (=So comments > comments > hidden is actually comments > hidden.)

@ImprovedTube
Copy link
Member

Currently going back is super annoying with having to click two times

search input need not overlap the back button.

^ would go Up in hierarchy.

often will be the same as back / one might be enough.

(while we should support tags (multiple categories for on feature)

below_player_screenshot
below_player_pip
below_player_loop, those three are [Extra buttons below the player] in two places,

)

@ImprovedTube ImprovedTube merged commit 0747f5f into code-charity:master Apr 27, 2024
@ImprovedTube
Copy link
Member

search.js L37
Uncaught TypeError: Cannot read properties of undefined (reading 'value')
satus.js L268 context: index.html
Uncaught (in promise) TypeError: Cannot read properties of null (reading 'appendChild')

@raszpl raszpl deleted the patch-11 branch April 28, 2024 12:26
@raszpl
Copy link
Contributor Author

raszpl commented Apr 28, 2024

how? I just checked out current master and no errors

@ImprovedTube
Copy link
Member

idk

@ImprovedTube
Copy link
Member

Currently going back is super annoying with having to click two times

search input need not overlap the back button.

And it could be focused on all pages🤩

Before the input would disappear when empty and on a category page, did that have to change? or should can we get that for now?

@raszpl
Copy link
Contributor Author

raszpl commented Apr 29, 2024

maybe it should be possible? (if satus.js is a framework)

Yes, with much more work it could be redesigned, Ill leave that to someone else :)

yet semantically it points to potential improvements: appearance elements (comments, sidebar, header) have the option to hide them completely.

afaik sidebar doesnt

So for the most precise UX hiding any of them completely should would grey out the element on the appearance screen and lock all the suboptions. (=So comments > comments > hidden is actually comments > hidden.)

if you grey out comments how will you re-enable them?

let self = this,
value = this.value.trim();
if (value) extension.search = value;

there is no need for that check, value is declared one line above as this.value, 'this' is Input element, those are guaranteed by html standard https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement
"value
A string that represents the current value of the control. If the user enters a value different from the value expected, this may return an empty string."

to have string .value. What I meant is How did you trigger that error :), because I cant trigger any. Is it possible you didnt restart full extension before testing? or didnt download all the changed files?
this is master snapshot from yesterday before this change, works without throwing any errors
youtube-master (7).zip

Currently going back is super annoying with having to click two times

search input need not overlap the back button.

more clutter

And it could be focused on all pages🤩

isnt it enough with search being active when opening Options Popup?

Before the input would disappear when empty and on a category page, did that have to change? or should can we get that for now?

Imo Options Popup starting with Search active is great, and the way it works now with empty query keeping it open while showing main skeleton.js is also great.
But as soon as user clicks one of the Categories its a clear sign he doesnt want to search and it should deactivate + opening search while in one of the categories should blur the bottom window even with empty search results so its clear to the user Search is active.
so:
start - search text-Input active, skeleton.js rendered below
enter category - search closed
click search while in any category - search active, below blurred regardless if any search results

@ImprovedTube
Copy link
Member

ImprovedTube commented Apr 29, 2024

hi! @raszpl and thanks!!

possible you didnt restart full extension

sorry if so!

i'll just say this here again, just in case you invest more time here.

while we should support tags (multiple categories for on feature)

(The 9 red buttons also are just "tags" 😅 Doesn't matter if something is our 1st-, 2nd- or 3rd-level category)

@ImprovedTube
Copy link
Member

ImprovedTube commented Apr 29, 2024

enough with search being active when opening Options Popup

yes, just because (at mine) search currently stayed visible now in the categories, unlike before.
didn't figure every change yet.
And i trust your choices will be improvements.


And it could be focused on all pages🤩

oh, and what this actually made me think of, that search could still listen to keyboard input while in a path since we don't need keyboard input.

...except😅 maybe tab, space, page-up, page-down & arrows .
And + btw, advanced thoughts

afaik sidebar doesnt

until i added "hide sidebar" in the end😅.

(

if you grey out comments how will you re-enable them?

Yes, *i guess*, then the blocks would have a red X or Trashbin icon at the parent level "appearance". (except player unless somebody wants transcript only).
Aand in future this minimap could be included on the front screen. (At least when calling the extension while on youtube.com. While maybe we can also run HTMLMediaElement features on all websites in future. Which also would be relevant.
))

)

@raszpl
Copy link
Contributor Author

raszpl commented Apr 29, 2024

enough with search being active when opening Options Popup

yes, just because (at mine) search currently stayed visible now in the categories, unlike before.

Weird, I could have sworn it was working like that before :o I just tested 4.823 and you are totally right! I had to change something, didnt notice, and later convinced myself it was like that from the start :] Ill fix that later.

oh, and what this actually made me think of, that it search could still listen to keyboard input while invisible, since we dont need keyboard input.

That is clever, but confusing for people not expecting it, and undiscoverable.

Yes, i guess, then the blocks would have a red X or Trashbin icon at the parent level "appearance". (except player unless somebody wants transcript only).

oh, thats easy to add. Dont know about red X, but something subtle to signal to the user that this whole category has no active options would be nice.

@ImprovedTube
Copy link
Member

ImprovedTube commented Apr 30, 2024

Dont know about red X

🚫. Yes, while semantically / structurally making sense, i assume the few features hiding a whole sections most arent used very often in improvedtube. ( should start counting: #1972 (comment) )


Trashbin

(while i mentioned this word in several threads/thoughts about structure. Active features and all features could be two icons on the front screen/always: my active features[🧰my toolbox & 🗑️trashbin]. Features to remove-clean-reduce or revert-undo youtube could have their own list (https://chromewebstore.google.com/detail/unhook-remove-youtube-rec/khncfooichmfjbepaaaebmommgaepoid ) like a search for remove|hide|block & not enabled, ordered by popularity and recency(new features), starting with youtube's experiment flags set to true, that the extension never heard of yet. With up to 730 in the end that could be disabled too but might be [known] marginal https://docs.google.com/spreadsheets/d/1GidvMduxTl6jXpDCKj-sOPg8KSqDfCYO2OlCdBADaSI/edit?pli=1#gid=0 )

@ImprovedTube
Copy link
Member

That is clever, but confusing for people not expecting it, and undiscoverable.

*with a little layout change search can always stay visible too.

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.

None yet

2 participants