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

Introducing bookmarks #1095

Merged
merged 7 commits into from
Oct 6, 2024
Merged

Introducing bookmarks #1095

merged 7 commits into from
Oct 6, 2024

Conversation

BentiGorlich
Copy link
Member

In this massive commit bookmarks are added. A user can create multiple bookmarking lists and add entries, posts and comments to them. They are all shown in one view when looking at the list.

UI

Add to default button

Entries, posts and comments now have a button to add a bookmark to the default bookmark list:

grafik

Add to list button

They all have the option to add them to a specific bookmark list in the dropdown menu:

grafik

Get to the list view

To get to the new view there is a new button in the profile dropdown:

grafik

The new list view

There is a new view for managing ones bookmark lists:

grafik

Looking at the content in a list:

When looking at the content of a bookmark list all types can be viewed simultaneously in the same list:

grafik

API

I already added the API, but it might not work correctly, as I didn't test it (might be possible to do so without an app to test it via test libs, but I didn't bother...)

There are some new scopes: user:bookmark, user:bookmark:add, user:bookmark:remove, user:bookmark:list, user:bookmark:list:read, user:bookmark:list:edit and user:bookmark:list:delete. I do not know if I forgot a few places for them to be added though

Extras

One point that is easy to miss: the caching of the twig components EntryCommentsNestedComponent and PostCommentsNestedComponent was removed (by not using the caching template). It is not really that useful (cache gets cleared as soon as anyone votes on it) and was getting in the way of rendering. That may later be reverted


Closes #46

@BentiGorlich BentiGorlich added enhancement New feature or request frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end backend Backend related issues and pull requests api API related issues and pull requests labels Sep 8, 2024
@BentiGorlich BentiGorlich added this to the v1.8.0 milestone Sep 8, 2024
@BentiGorlich BentiGorlich self-assigned this Sep 8, 2024
@melroy89

This comment was marked as outdated.

@melroy89
Copy link
Member

melroy89 commented Sep 10, 2024

Ow.. it's only when you are logged in?.. Makes since I guess.

@melroy89
Copy link
Member

melroy89 commented Sep 10, 2024

When adding an item to the (default) bookmark from the drop-down menu. Should it not also be making the bookmark icon "active" of the icon you still see on the thread itself (next to the More button).

image

In fact, this icon will become active when I fully refresh the page, manually..

image

@melroy89
Copy link
Member

"Bookmark list" menu item will not become bold (aka active) when it should be:

image

@melroy89
Copy link
Member

Would it be possible to uppercase the default name to Default?

image

@melroy89
Copy link
Member

Removing the item from the bookmark will not remove the item from the list:

image

Only after I refreshed the page manually, the item is removed.

@BentiGorlich
Copy link
Member Author

I fixed the minor points.
Just to summarize yours:

  1. when adding something to a specific list via dropdown the "add to default" icon will not be changed to active
  2. When viewing a list and removing an item from the list it is not removed until a page refresh

Answers:

  1. could be done and wouldn't be to hard, though we do it via replacing html of some anchors we have and with that it is way harder to do it, since they are 2 separate controls
  2. that could of course be done, but it is not something I'd like it to do actually. What if you want to just move it to another list and because you didn't knew mbin behaved like this you remove it first and then its gone...

@melroy89
Copy link
Member

I fixed the minor points. Just to summarize yours:

1. when adding something to a specific list via dropdown the "add to default" icon will not be changed to active

2. When viewing a list and removing an item from the list it is not removed until a page refresh

Answers:

1. could be done and wouldn't be to hard, though we do it via replacing html of some anchors we have and with that it is way harder to do it, since they are 2 separate controls

2. that could of course be done, but it is not something I'd like it to do actually. What if you want to just move it to another list and because you didn't knew mbin behaved like this you remove it first and then its gone...
  1. You can use JS to trigger the bookmark icon on top-level thread/comment to become active. I believe that is possible.
  2. I mean when you click the "Remove from all bookmark" button (which is the bookmark icon saying when active), the bookmark will be removed. So it's not moving to another list. In this case you can also use JS to just hide the thread/comment.

@BentiGorlich
Copy link
Member Author

  1. but I'd have to change the text and the link it goes to as well. So I'd have to implement a way to fetch this component from the server. My point is that it is not just the icon :D
  2. Ok yeah that makes sense, but only on the "remove all" button

@BentiGorlich BentiGorlich changed the base branch from main to dev/new_features September 13, 2024 07:14
@BentiGorlich
Copy link
Member Author

I addressed 1. but I am hesitent to implement 2. are you fine with letting the users decide whether they want 2. when this is released @melroy89 ?

@jwr1
Copy link
Member

jwr1 commented Sep 27, 2024

I think I'm with @BentiGorlich on this. I just checked, and even GitHub stars behave the same way; if you go to the star list in your profile and unstar one of the repositories, it still stays there until you actually reload the page (which works out in case you accidentally remove one of them).

melroy89 and others added 4 commits September 27, 2024 17:58
In this massive commit bookmarks are added. A user can create multiple bookmarking lists and add entries, posts and comments to them. They are all shown in one view when looking at the list.

One point that is easy to miss: the caching of the twig components `EntryCommentsNestedComponent` and `PostCommentsNestedComponent` was removed (by not using the caching template). It is not really that useful (cache gets cleared as soon as anyone votes on it) and was getting in the way of rendering. That may later be reverted
- change route paths from `lists` -> `bookmark-lists`
- when creating a default list because the user doesn't have one yet it will be called `Default`
- add a title and aria-description to the start icon (default list)
- make the header dropdown item be bold when it should be
- if one adds an item to the standard list the button for that specific list will update as well
- if adding an item to a specific list the default item will be updated as well
@melroy89
Copy link
Member

I think I'm with @BentiGorlich on this. I just checked, and even GitHub stars behave the same way; if you go to the star list in your profile and unstar one of the repositories, it still stays there until you actually reload the page (which works out in case you accidentally remove one of them).

Aha. I see. To be honest don't compare UX with Github, since if some interface can be improved on planet earth it would be all Microsoft products including github. ☺️

@melroy89
Copy link
Member

are you fine with letting the users decide whether they want 2. when this is released @melroy89 ?

Sure. It's a minor UX issue I think. Mainly confusing I believe.

Copy link
Member

@melroy89 melroy89 left a comment

Choose a reason for hiding this comment

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

Works!

@BentiGorlich BentiGorlich merged commit 7b8a9f2 into dev/new_features Oct 6, 2024
5 checks passed
@BentiGorlich BentiGorlich deleted the new/bookmarks branch October 6, 2024 12:16
BentiGorlich added a commit that referenced this pull request Oct 7, 2024
Co-authored-by: Melroy van den Berg <melroy@melroy.org>
BentiGorlich added a commit that referenced this pull request Oct 9, 2024
Co-authored-by: Melroy van den Berg <melroy@melroy.org>
BentiGorlich added a commit that referenced this pull request Oct 10, 2024
Co-authored-by: Melroy van den Berg <melroy@melroy.org>
BentiGorlich added a commit that referenced this pull request Nov 19, 2024
Co-authored-by: Melroy van den Berg <melroy@melroy.org>
BentiGorlich added a commit that referenced this pull request Nov 24, 2024
Co-authored-by: Melroy van den Berg <melroy@melroy.org>
BentiGorlich added a commit that referenced this pull request Dec 8, 2024
Co-authored-by: Melroy van den Berg <melroy@melroy.org>
BentiGorlich added a commit that referenced this pull request Dec 10, 2024
Co-authored-by: Melroy van den Berg <melroy@melroy.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API related issues and pull requests backend Backend related issues and pull requests enhancement New feature or request frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bookmark threads/posts
3 participants