-
-
Notifications
You must be signed in to change notification settings - Fork 743
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
Page bookmarks #5003
base: development
Are you sure you want to change the base?
Page bookmarks #5003
Conversation
8efa37b
to
21601ed
Compare
Reduces chance that the star is seen as a button or control.
21601ed
to
7896b7b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
…at/add-page-bookmarking
This comment was marked as duplicate.
This comment was marked as duplicate.
This feature differs quite a bit from what I imagine it to be. Lets take the following artistic masterpiece as a starting point (this applies to #1505 and #1492): ONLY when the user clicks on an empty search bar (imagine the search bar being empty in the pic above):
When the user starts to type into the search bar:
Debatable:
I dont see the need for:
|
Addressing #1492 is out of scope for this PR, IMO, unless we want to make it even bigger.
I disagree on this one. This sets a hard limit on the number of bookmarks, makes searching bookmarks harder, and is different from most browser (and in the case of YT for search history) behavior that people are most familiar with, where those are put at the top of the results.
I disagree with this one for the reason that it's a prohibitive UX. You don't often think to save a page before you actually click on it, which is going to lead to a good deal of frustration. You probably want it to be somewhere visually constant so that you can bookmark a page whenever you want to. See my discussion from the OP:
See answers to the above on why I disagree. In short, the cognitive model of browser bookmarking is one that users know about and are more effortlessly familiar with, but the model you're proposing is one that users will have to put in some work to learning and understanding the behaviors you've laid out, as it's a novel (at least to my knowledge) system and ruleset.
I can see the argument for this one. I would be open to having the modal close action for this one be to save rather than cancel (similar to Firefox), but I do worry that's not as intuitive given that differs from our other modals. Alternatively, we could have bookmarking not open the modal by default, but a toast pops up presenting you the option to edit it (otherwise, you can just click the icon again). |
This comment was marked as outdated.
This comment was marked as outdated.
Included that one just to make the examples easier to understand
Ah i didnt know. How does YT search history work? Could you maybe expand some more on the how browsers tackles bookmarks and how we do a similar job of it (i havent used browser bookmarks for a long time)
Ah ok I think I understand
👍 im fine with it but please provided some examples of the familiar behaviors like requested above.
After testing some more i withdraw this. I think its very valuable to search for something with a filter applied and bookmark it with an alias so you exactly know what it is |
Incidentally, I'm thinking about us moving the star icon to the left of the label, adding a magnifying glass by regular results, adding a loop icon for history results, and adding remove icon or text button on the right side for history results.
Some behaviors vary based off of the specific browser and what settings you have enabled, but at least in every major browser, bookmarks and search history are prioritized over regular results by default. See discussion here on Chromium's behavior for example. As to which takes precedence over which, I think it varies. In Firefox, judging from a few minutes of testing, bookmarks and search history are equal, with relevance, closeness of order, and time of search seeming to be the apparent factors of sorting in order of priority descending.
This is grabbed from Firefox: |
…at/add-page-bookmarking
A few comments after trying it out briefly
|
From the Web side, the idea is that the For mobile, I honestly didn't think about it needing something different, and I'm glad you pointed it out. Interestingly, it seems like most mobile web browsers I'm testing with don't actually show a modal by default at all. Firefox Mobile saves the bookmark with a toast in one click, then lets you either re-click the icon to remove, press the toast to Edit, or go to a Bookmark Settings to edit it after you find it visually in the list. What about mobile one-click save, toast on save that opens the Modal, and re-click on a bookmarked video to open the modal?
This one is interesting. I'm guessing you're referring to the custom routes where we show the playlist name or video title and not the
This one is interesting as well. I could see it being convenient for the use case of "get rid of this video everywhere", having them linked like this, but I'm not sure if that would ever be undesirable. Maybe I'll add this one with an undo toast.
This is in line with how browsers act, so I don't think it would be surprising / a bad UX as a user necessarily. I'll revisit this one when we have the route-specific icon, because in such a case having multiple same-named bookmarks would still be understandable if they're different pages for the same channel (e.g., their UULF playlist, their channel page, etc.,) or different gradients of the same content (e.g., a "GOAT music" video, a "GOAT music" playlist). |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…at/add-page-bookmarking
Conflicts have been resolved. A maintainer will review the pull request shortly. |
2dc496e
to
e5b5b43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Playlists page & single playlist page (remote & local) bookmark entries got different icons?
Could use better default titles, e.g. playlist to have channel name when exists like #5226, same for videos
return currentBookmarkAdjustment + this.pageBookmarks.filter((pageBookmark) => pageBookmark.name === this.name).length | ||
}, | ||
duplicateNameMessage: function () { | ||
return this.$tc('Page Bookmark["There is {count} other bookmark with the same name."]', this.duplicateNameCount, { count: this.duplicateNameCount }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remove the dot in key and it'a much easier to type (rant)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, I think having the key match the EN-US content as close as possible is best, even for minor things like punctuation, which help communicate tone
@@ -167,12 +167,39 @@ class Playlists { | |||
} | |||
} | |||
|
|||
class SearchHistory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should rename all related code to page bookmark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is that we could have persistent keyword search history entry logic here in the future (see Future PRs
in OP), and page bookmarks are a type of search history.
Yes, see likely implementation for the inline user playlist icon for the motivation on that.
If we don't like the current titles we're using, could we modify the |
Hm, any suggestions? What if we made the Channels tab user-check (separate PR, of course)? Here's what that would look like: (Side-note: regardless of this, we may need to create a PR normalizing icon width while preserving the original proportions, both here and in |
It might help but playlist (at least user playlist) should still use |
@PikachuEXE How about this? It's a bit odd since we're using filled versus unfilled to signify Quick Bookmark icon versus non-Quick Bookmark, but that's in a separate enough context, so I think it should be understandable here. |
Then the problem being not being able to remember solid/normal icons means local or remote playlist |
Page bookmarks
Pull Request Type
Related issue
closes #1505
stopgap/weak alternative for #4594 and #312
provides setup for #1492
Description
document.title
, playlist name, or search query depending on routeScreenshots
Testing
General Settings > Enable Search Settings
is disabledTheme Settings > Match Top Bar with Main Color
enabled (secondary color theme is used as fill)Desktop
Additional context
Future PRs
search-history.db
entries withisBookmark: false
set. Edit: probably should just leverage the existing search cache as the data source & existing access methods, but we can reuse the UI patterns added in this PR. I can revert 392fa75, the commit I added specifically to better accommodate the possibility of a generic permanent search history through thesearch-history.db
, if desired. It depends on whether we want to support a permanent search history and/or one with time-based expiry rules going forward.Aspects open for discussion
- FreeTube
part in much of the default bookmark names, even though it's a bit ugly, because it helps to further visually (& search-wise) demarcate bookmarks, and is thus a good default.spellcheck
on for the bookmark name insertionft-input
because we do it elsewhere for most of our inputs, even though it's a bit ugly, especially whenFreeTube
is underlined in red for most users. This one is debatable, but if I change it for this one, I'm also inclined to change it for other instances as well (e.g., User Playlist name insertion, profile name insertion)./about
are bookmarkable, even though they're equally accessible through the side nav. This might seem unnecessary, but having controls on the top nav phase in and out of being active seems like a poor & confusing choice in comparison. Having it be omnipresent like all of our other top nav icons, and communicating that any route works for it, makes the new feature more apparent and easier to understand/use.