-
-
Notifications
You must be signed in to change notification settings - Fork 171
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
Implement Highlighted Categories (aka like button) #1173
base: dev
Are you sure you want to change the base?
Conversation
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.
This looks overall good, sweet and simple. I don't have much to complain about.
I'm a bit iffy on the "highlighted" term, but I don't really have a better one coming to mind at the moment.
Regarding UI, I'd just add a checkmark below "pin this category" that makes the selected cat the highlighted one. Ofc, you can only do this with static categories. In the index, i think you can add the "like" button to the javascript-created thumbnails fairly easily, that's in a dedicated function. You need however to know the "initial" state of the like button when the index loads (aka whether an ID is in the highlighted category or not). |
Co-authored-by: Difegue <[email protected]>
Co-authored-by: Difegue <[email protected]>
Co-authored-by: Difegue <[email protected]>
Yeah I'm not too sure about the naming either, we can sit on that for a while, see if anyone has thoughts, or ask an AI for ideas. I wouldn't mind if we just gave it a Japanese phrase either. I'm sure the front end work isn't difficult, I just need to spend some good time learning the basic principles and HTML/CSS/JS structure/DSL and speaking the frontend language. Right now I don't know what I don't know or what I should know, or if a term is used in a informal or technical setting. |
Fair enough - Maybe someone will come up with a better name by the time we wrap up the UI! |
Implement bookmark and bookmark link
Ok I have a first draft down. I'm going to test this on my server for a bit and clean up the code (also I really want to start using it lol). A lot of sync-related bugs were squashed, but I'm sure there are still more.. I'll see how it works in prod-like environments. Also added some python API integration tests on my side. Didn't do much code standardization, I figure more changes will be made. I've decided to go with the name "bookmark" for the frontend, and "bookmark link" for the backend. The word highlight is too confusing, and this backend functionality is a conceptually abstract; most websites don't allow you to flexibly change where the bookmark points to. It's like youtube letting you choose which playlist the like button saves videos to. A readonly user would never see the "bookmark link" terminology; they would only interact with the bookmark button. (We could also go for "like", "star", "love", "save", and replace "link" with "reference" or "connect". I realized midway the repo uses fontawesome which supplies icons, so we don't need to cook up in-house icons. However, "save" and "star" are overloaded terms, and "like/love" generally implies the notion of a "dislike". Another idea is "quick save" or something along the lines of that...) Now, for functionality: the bookmark icon/button is implemented in three locations mainly: categories, reader, and index, with helpers in LRR and server. Generally, the bookmark category ID will be fetched on initialization and put in local storage. Categories page: Enables changing which category the bookmark toggle points to, and whether the bookmark toggle should exist. Reader: adds a bookmark icon to the Index page: adds a bookmark icon to the bottom left of each image thumbnail. For every archive, their icons are synced; e.g., when the same archive appears in carousel and in datatables. Also, these icons are synced with the context menu categories section. If you unbookmark/bookmark an archive through any channel, all other indications will be updated. Also, this bookmark icon will show only when the bookmark is configured. I'm not sure about the way we check if an archive is bookmarked or not, I don't want to make many api calls though Index page (compact): prepends a checkbox col before the title col. Likewise this checkbox column is synced with all other channels. When the bookmark is not configured, this column will display off and be disabled from toggling. Maybe we'd want to disable this column instead, but I haven't gotten that to work. I still don't really understand the datatables-related code. |
Add bookmark shortcut
@@ -48,7 +48,7 @@ | |||
|
|||
</head> | |||
|
|||
<body> | |||
<body data-user-logged="[% userlogged %]"> |
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.
Since userlogged is a server-side variable, it's not exposed to the client so the client doesn't really know whether the user is logged in.
Toggleability of bookmark icon should be enabled only if the user is logged in (i.e. user has "write" permissions). In readonly mode (i.e. user viewing site with no-fun-mode disabled), bookmark icons and checkboxes should not be toggleable. This decision requires exposing the logged-in status to the client somewhat, hence the use of html data attributes here
Add i18n (zh) for bookmarks
templates/footer.html.tt2
Outdated
</p> | ||
|
||
<!-- i18n client-side access --> |
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.
Decided to put i18n translations that are dynamically required by clients in this central location and made accessible by common.js, to avoid making script initializations in every tt2 file that corresponds to a js file that dynamically generates elts with translatable text
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.
avoid making script initializations in every tt2 file that corresponds to a js file that dynamically generates elts with translatable text
How did you figure out that was what I was working on for frontend translations? 🫠
I've pushed my WIP here for reference -- 2eefd23
I'm not sure if I prefer putting it all in the footer -- It feels less clear when working on the codebase that this is where the glue for frontend TLs would end up.
Also, this requires double-declaring every string in common.js, which is going to add a lot of heft to that file.
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.
(I'm also still undecided on how to work with placeholders in localized text.. The backend didn't have this issue for obvious reason, but I need to move everything to JS template strings with ${...}
)
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.
Ah I didn't know how the i18n system worked initially, when I found it was generated by the server I figured this problem would come up sooner or later 😅
Yeah I wasn't too excited about this approach either, If you're working on this part then I'll just use yours when it's implemented, then I can do away with this awkward workaround
@@ -1028,6 +1031,8 @@ msgstr "档案概览" | |||
msgid "FullScreen" | |||
msgstr "全屏" | |||
|
|||
msgid "Toggle Bookmark" | |||
msgstr "切换书签" | |||
# ------End of Reader.html.tt2------ |
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.
Kept the Chinese translations minimal, but alternative suggestions are welcome.
const icon = e.currentTarget; | ||
const id = icon.id; | ||
|
||
// TODO: Considering removing this toast |
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.
These types of toasts might be too noisy, so I wouldn't mind if we remove them, or replace with something else (same with adding/removing archives to/from categories)
Issue #1129
Completed:
Ongoing implementation of the like button backend and frontend logic. To strive for a neutral term, "highlight" is tentatively used (English is hard...) but everything so far, naming and logic, can be changed.
A static category is "highlighted" if archives in the index and reader page can be added and removed from this category via a 1-click icon toggle. Only one category may be highlighted at any time. The assignment and un-assignment of a highlight may be done via the API or on the browser via the config/categories page. If a highlighted category is deleted, the highlight will point to an empty string.
Additionally, all new installs of LRR will create a highlighted category called "Favorites". In such cases where the category corresponds to liked archives, the toggle is equivalent to a like button.
As for the UI I am thinking of doing something like this (but move Highlight row below "Delete Category"):
Expanding it shows a list of static categories + "No Category". Choosing a static category moves the highlight to that category. If a highlight is assigned, "No Category" will instead be replaced with the name of the highlighted category.
There is quite a bit left to do, but I think now is a good time to checkpoint. There is settling on a UI, and icons to replace the placeholder toggle icons. If this backend is approved, we can move forward with API documentation.
This is also a request for feedback/available help, as I haven't decided on the design of the frontend, and I am not familiar with *cough* basically any frontend. Now these are things I can eventually learn and come up with something, but I'm expecting this will take a while.