Feature: Bookmarks #403

levinoeninckx · 2025-02-25T22:09:00Z

No description provided.

Add bookmarService Add bookmark page

Use this hook because of the need to use javascript to access localStorage

linkdotnet · 2025-02-26T19:18:05Z

That is already a good start - of course the simple button is a bit rough, but I get your idea.
A small detail: The "Bookmarks" shouldn't be part of the RSS Feed:

I showed cases that because adding another icon on the navbar might be just too much. Maybe we can merge the Booksmarks with the Archive into dropdown. I am really open to new ideas here.

For the icon. We can do one of those two approaches. Number 1: The simple one:

Use a website like https://heroicons.com/outline and copy&paste the SVG into a button and we are done. Or better: https://flowbite.com/icons/ (they have more)
What I outlined earlier: https://github.com/linkdotnet/Blog/blob/master/src/LinkDotNet.Blog.Web/wwwroot/css/fonts/README.md
Go to https://icomoon.io/app/#/select, upload the "blog.json" and add the two new icons and download the the new woff files and so on.

The further I think of it, probably the more I want to replace everything with approach 1 as it is just way too annoying with approach 2.

levinoeninckx · 2025-02-26T20:03:15Z

That is already a good start - of course the simple button is a bit rough, but I get your idea. A small detail: The "Bookmarks" shouldn't be part of the RSS Feed:

I showed cases that because adding another icon on the navbar might be just too much. Maybe we can merge the Booksmarks with the Archive into dropdown. I am really open to new ideas here.

For the icon. We can do one of those two approaches. Number 1: The simple one:
1. Use a website like https://heroicons.com/outline and copy&paste the SVG into a button and we are done. Or better: https://flowbite.com/icons/ (they have more)

2. What I outlined earlier: https://github.com/linkdotnet/Blog/blob/master/src/LinkDotNet.Blog.Web/wwwroot/css/fonts/README.md
   Go to https://icomoon.io/app/#/select, upload the "blog.json" and add the two new icons and download the the new woff files and so on.
The further I think of it, probably the more I want to replace everything with approach 1 as it is just way too annoying with approach 2.

Okay, I can create another collapsable navbar item (maybe 'collection'). I'll fix the icon and replace it with the button
ps: my bad, already thought it was weird to put it together with RSS :p

linkdotnet · 2025-02-26T20:09:57Z

Nah don't worry - just a detail. If you have other options/opinions let me know. I don't want to overload the navigation bar. I already feel there is too much stuff inside.

levinoeninckx · 2025-02-26T20:16:10Z

I currently have no clue, tbh I feel like the navbar isn't overloaded at all (yet) so personally think it is fine to combine it with archive.
But it could be easily chanaged, maybe if a sidebar of some sort is added in the future.
Maybe some sort of side panel that slides in, showing the titles...

linkdotnet · 2025-02-26T20:18:28Z

I currently have no clue, tbh I feel like the navbar isn't overloaded at all (yet) so personally think it is fine to combine it with archive. But it could be easily chanaged, maybe if a sidebar of some sort is added in the future. Maybe some sort of side panel that slides in, showing the titles...

Maybe I am overreacting :D In any case - we can try without moving it into a category with the archive. Grouping it later on is done in no time

linkdotnet · 2025-02-27T19:36:19Z

src/LinkDotNet.Blog.Web/Features/Bookmarks/Components/BookmarkButton.razor

+<button  @onclick="OnClickHandler">
+	@if (!IsBookmarked)
+	{
+		<svg class="w-6 h-6 text-gray-800 dark:text-white" aria-hidden="true" xmlns="http://www.w3.org/2000/svg" width="24" height="24" fill="none" viewBox="0 0 24 24">


Good start - as it is from Flowbite, it still has a lot of tailwindcss stuff inside.

Also it needs a bit more styling with tailwindcss (adding some styling on the button class)

I have removed the button background as well as the border and added a hover effect for the icon

linkdotnet · 2025-02-27T19:39:29Z

src/LinkDotNet.Blog.Web/Features/Bookmarks/BookmarkService.cs

+        return bookmarks.Contains(postId);
+    }
+
+    public async Task<IReadOnlyList<BlogPost>> GetBookmarkedPosts()


I would refactor that method or the general approach a bit better.
The Bookmark service should only take care of the LocalStorage.

The Bookmark-Page is responsible to load all the data via the repository. There should be no need of a DbContextFactory - keep in mind that this blog also works with RavenDb, MongoDb, ...

YOu can have a look on the ArchivePage or SearchPage.

I reimplemented the GetBookmarkedPosts method, apologies did not consider that it can have different databases, im using the Repository in the bookmarks page now

linkdotnet · 2025-02-27T19:40:48Z

src/LinkDotNet.Blog.Web/Features/Home/Components/NavMenu.razor

-                @if (Configuration.Value.IsAboutMeEnabled)
+	            <li><a class="nav-link" href="/"><i class="home"></i> Home</a></li>
+	            <li><a class="nav-link" href="/archive"><i class="books"></i> Archive</a></li>
+	            <li><a class="dropdown-item" href="/bookmarks">Bookmarks</a></li>


We can try to add an icon here instead as well. It also needs the "nav-link" class, otherwise it is misaligned.

Okay, i'll add it and whoops overlooked that from copy pasting it

levinoeninckx · 2025-02-28T18:45:18Z

I also added a bit of padding to the Bookmarks page

linkdotnet · 2025-03-01T08:59:27Z

src/LinkDotNet.Blog.Web/Features/Bookmarks/BookmarkService.cs

+
+    public async Task<bool> IsBookMarked(string postId)
+    {
+        if(string.IsNullOrEmpty(postId)) throw new ArgumentException(nameof(postId));


Suggested change

if(string.IsNullOrEmpty(postId)) throw new ArgumentException(nameof(postId));

ArgumentException.ThrowIfNullOrEmpty(postId);

linkdotnet · 2025-03-01T09:00:23Z

src/LinkDotNet.Blog.Web/Features/Bookmarks/BookmarkService.cs

+    public async Task ToggleBookmark(string postId)
+    {
+        await InitializeIfNotExists();
+        if(string.IsNullOrEmpty(postId)) throw new ArgumentException(nameof(postId));


The same here - plus: The sanity check should be the first line in this code. No need to call InitializeIfNotExists when postId is not set.

linkdotnet · 2025-03-01T09:02:43Z

src/LinkDotNet.Blog.Web/Features/Home/Index.razor

@@ -14,6 +15,7 @@
 @inject IOptions<Introduction> Introduction
 @inject IOptions<ApplicationConfiguration> AppConfiguration
 @inject NavigationManager NavigationManager
+@inject IBookmarkService BookmarkService


This seems not needed - can be removed

src/LinkDotNet.Blog.Web/Features/Bookmarks/Components/BookmarkButton.razor

linkdotnet · 2025-03-01T09:07:30Z

src/LinkDotNet.Blog.Web/Features/Bookmarks/IBookmarkService.cs

+
+public interface IBookmarkService
+{
+    public Task<bool> IsBookMarked(string postId);


I would also write IsBookmarked as you did not use capital M for the other methods.

linkdotnet

Thanks for sticking with me. There are some smaller things I would like to address.

linkdotnet · 2025-03-01T21:09:06Z

src/LinkDotNet.Blog.Web/Features/Bookmarks/Components/BookmarkButton.razor

+	}
+</button>
+
+<style>


We either could move this into BookmarkButton.razor.css or on the global css file.

src/LinkDotNet.Blog.Web/Features/Bookmarks/Components/BookmarkButton.razor

linkdotnet · 2025-03-01T21:10:38Z

src/LinkDotNet.Blog.Web/Features/Bookmarks/Bookmarks.razor

+@inject IBookmarkService BookmarkService
+@inject IRepository<BlogPost> BlogPostRepository;
+
+<section>


Could we align this a bit better to other pages like the archive?

<div class="container"> <h3 class="pb-3 fw-bold">Bookmarks</h3> ...

linkdotnet · 2025-03-01T21:11:13Z

src/LinkDotNet.Blog.Web/Features/Bookmarks/Bookmarks.razor

+	}
+</section>
+<style>
+	section {


Given the idea from above, this could be deleted

linkdotnet · 2025-03-01T21:11:58Z

src/LinkDotNet.Blog.Web/Features/Bookmarks/Bookmarks.razor

+	protected override async Task OnAfterRenderAsync(bool firstRender)
+	{
+		var ids = await BookmarkService.GetBookmarkedPostIds();
+		bookmarkedPosts = await BlogPostRepository.GetAllByProjectionAsync(post => post, post => ids.Contains(post.Id));


nitpick: We could make a small sanity check if there are any ids. No need to go to the database if ids.Count == 0

linkdotnet · 2025-03-01T21:16:04Z

src/LinkDotNet.Blog.Web/Features/Bookmarks/BookmarkService.cs

+        ArgumentException.ThrowIfNullOrEmpty(postId);
+        await InitializeIfNotExists();
+
+        if (await IsBookmarked(postId))


The two branches do a lot in common. Only the add/remove is different. Therefore the getItemand SetItem are common

linkdotnet · 2025-03-01T21:16:57Z

src/LinkDotNet.Blog.Web/Features/Components/ShortBlogPost.razor

-			<h2></h2>
+			<div class="header">
+				<h1>@BlogPost.Title</h1>
+				<BookmarkButton IsBookmarked="isBookmarked" OnClick="async () => { await BookmarkService.ToggleBookmark(BlogPost.Id); StateHasChanged(); }"></BookmarkButton>


As this does more than one thing, it might be nice to have it inside a function rather than a lambda here.

linkdotnet · 2025-03-01T21:18:46Z

src/LinkDotNet.Blog.Web/Features/ShowBlogPost/ShowBlogPostPage.razor

@@ -129,6 +134,10 @@ else if (BlogPost is not null)
    {
        await JsRuntime.InvokeVoidAsync("hljs.highlightAll");
        _ = UserRecordService.StoreUserRecordAsync();
+
+        ArgumentNullException.ThrowIfNull(BlogPost);


As this isn't an argument, the ArgumentNullException doesn't really apply.
Rather an if(BlogPost is not null) - if the user navigates to a page with a non-existing ID then we get an exception

levinoeninckx · 2025-03-01T21:26:31Z

Thanks for sticking with me. There are some smaller things I would like to address.

Np It's fun to work on something else aside from my job :D. I'll adress the last few things when I get the chance

linkdotnet · 2025-03-02T16:04:54Z

Some tests are failing (the Blazor Unit testing) mainly because of new dependencies that aren't mocked or provided in the respective test suite

levinoeninckx · 2025-03-02T16:07:41Z

Some tests are failing (the Blazor Unit testing) mainly because of new dependencies that aren't mocked or provided in the respective test suite

Yes, I'm not familiar with the bUnit library (yet), I'm gonna try look into it later today and then try fixing those tests, if that is fine

linkdotnet · 2025-03-02T16:11:32Z

Some tests are failing (the Blazor Unit testing) mainly because of new dependencies that aren't mocked or provided in the respective test suite

Yes, I'm not familiar with the bUnit library (yet), I'm gonna try look into it later today and then try fixing those tests, if that is fine

Don't worry - I can also take over that part if you want. But of course, I don't want to push myself on top of your good work!

levinoeninckx · 2025-03-03T18:51:47Z

@linkdotnet I have been messing around with bUnit to fix the failing tests but I'm having difficulties.
For some reason when I add the IBookmarkService to the services collection on a test and mock the ILocalStorage service I get some sort of error from Xunit.
I'm not really sure what to do about this so I'm asking for some help on this
PS. The error returned is not even readable since it is even over 100 lines so no real usefull information

linkdotnet · 2025-03-03T18:54:32Z

No worries - I can take over that part.

linkdotnet · 2025-03-03T19:01:50Z

PS. The error returned is not even readable since it is even over 100 lines so no real usefull information

That might be from bUnit itself. Could that I am maintainer of that library as well. Glad to have more work :D

levinoeninckx · 2025-03-03T19:04:33Z

PS. The error returned is not even readable since it is even over 100 lines so no real usefull information

That might be from bUnit itself. Could that I am maintainer of that library as well. Glad to have more work :D

No clue, tried to read the entire log file that got spewed out and it starts at xUnit in the stacktrace and goes all the way down to the testrunner so not really sure 🤷‍♂️

linkdotnet · 2025-03-03T19:08:36Z

PS. The error returned is not even readable since it is even over 100 lines so no real usefull information

That might be from bUnit itself. Could that I am maintainer of that library as well. Glad to have more work :D

No clue, tried to read the entire log file that got spewed out and it starts at xUnit in the stacktrace and goes all the way down to the testrunner so not really sure 🤷‍♂️

Looks like an StackOverflow due to StateHasChanged inside OnAfterRender. Something we might fix on our site (bUnit)

linkdotnet · 2025-03-03T19:13:05Z

Caused by: bUnit-dev/bUnit#1682

linkdotnet · 2025-03-03T19:16:26Z

I will also fine-tune a bit the icon:

The picture is taken from my current blog posts.

linkdotnet · 2025-03-07T15:31:01Z

@levinoeninckx the issue with stackoverflow was mainly because StateHasChanged was called inside OnAfterRenderAsync which lead to another cycle of StateHasChanged inside OnAfterRenderAsync.... ;)

linkdotnet · 2025-03-07T16:09:14Z

So I added some more tests, fixed some behavior and did some small restyling.

Is there anything open from your side, otherwise that goes to master!

Thanks for the hard work! @levinoeninckx

levinoeninckx · 2025-03-07T16:11:17Z

Nope all good! @linkdotnet

linkdotnet · 2025-03-07T16:11:50Z

Awesome - then merge it is!

Levi Noeninckx added 9 commits February 23, 2025 20:39

implement bookmarks

5428428

Add bookmarService Add bookmark page

clear storage on startup when in development

4244410

add conditional bookmark buttons & use onAfterRender lifecycle hook

b396e1c

Use this hook because of the need to use javascript to access localStorage

move bookmarks link to rss

a5df71e

add bookmark button to post page

6bdb0d8

remove bookmark icon and change position

890744d

remove localStorage.js

720ef5d

remove unused dependencies

9f75386

Fix CI/CD build issues

4a2d9ac

Levi Noeninckx added 3 commits February 26, 2025 21:35

Create & use BookmarkButton, update bookmarkService

cf5922a

move bookmarks nav out of rss

9f3b6a1

refactor BookmarkService

3badad6

linkdotnet reviewed Feb 27, 2025

View reviewed changes

Levi Noeninckx added 5 commits February 28, 2025 17:37

refactor bookmarkservice

a697224

update navmenu

220eb5a

restyle bookmark button and add icon to navMenu

8fef145

add extra styling to bookmark button

7233cab

add padding to bookmarks page

1910223

Levi Noeninckx added 2 commits February 28, 2025 20:11

add unit tests for BookmarkService

7548d03

change exception type

3eeca43

linkdotnet reviewed Mar 1, 2025

View reviewed changes

src/LinkDotNet.Blog.Web/Features/Bookmarks/Components/BookmarkButton.razor Outdated Show resolved Hide resolved

linkdotnet reviewed Mar 1, 2025

View reviewed changes

refactor code

91bc212

linkdotnet reviewed Mar 1, 2025

View reviewed changes

Levi Noeninckx added 2 commits March 2, 2025 13:15

move BookmarkButton css to seperate file

0304be3

refactor

71533ae

linkdotnet added 2 commits March 7, 2025 15:24

Only update initially

e590c40

fix: Tests

52f3a8a

linkdotnet added 4 commits March 7, 2025 16:43

Added tests

7b4c994

Updated bookmark implementation and icons

22fc52b

refactor: Remove unused objects

910707f

fix: Styling alignments

e7468e2

fix: Remove debug stuff

efa0e0f

linkdotnet merged commit bbd1891 into linkdotnet:master Mar 7, 2025

	if(string.IsNullOrEmpty(postId)) throw new ArgumentException(nameof(postId));
	ArgumentException.ThrowIfNullOrEmpty(postId);

Feature: Bookmarks #403

Feature: Bookmarks #403

Conversation

levinoeninckx commented Feb 25, 2025

linkdotnet commented Feb 26, 2025 • edited Loading

levinoeninckx commented Feb 26, 2025 • edited Loading

linkdotnet commented Feb 26, 2025

levinoeninckx commented Feb 26, 2025

linkdotnet commented Feb 26, 2025

linkdotnet Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levinoeninckx commented Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linkdotnet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levinoeninckx commented Mar 1, 2025

linkdotnet commented Mar 2, 2025

levinoeninckx commented Mar 2, 2025

linkdotnet commented Mar 2, 2025

levinoeninckx commented Mar 3, 2025

linkdotnet commented Mar 3, 2025

linkdotnet commented Mar 3, 2025

levinoeninckx commented Mar 3, 2025

linkdotnet commented Mar 3, 2025

linkdotnet commented Mar 3, 2025

linkdotnet commented Mar 3, 2025

linkdotnet commented Mar 7, 2025

linkdotnet commented Mar 7, 2025

levinoeninckx commented Mar 7, 2025

linkdotnet commented Mar 7, 2025

linkdotnet commented Feb 26, 2025 •

edited

Loading

levinoeninckx commented Feb 26, 2025 •

edited

Loading

linkdotnet Feb 27, 2025 •

edited

Loading