-
Notifications
You must be signed in to change notification settings - Fork 345
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
client: Double click on title to mark entry as read #1469
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for selfoss canceled.
|
@@ -523,6 +523,7 @@ span.offline-count.diff { | |||
color: #999999; | |||
padding-top: 7px; | |||
padding-bottom: 7px; | |||
user-select: none; |
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 will make it impossible to select the title to copy/search it.
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.
That's intentional, because double clicks also mark text. However, thinking about it, why isn't event.preventDefault()
preventing that? I'll look into this whether there's a better solution, but first let us decide what to do with the delay (see below).
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.
Yes, I know, I usually do double click to select words.
@@ -444,7 +487,7 @@ export default function Item({ currentTime, item, selected, expanded, setNavExpa | |||
{/* title */} | |||
<h3 | |||
className="entry-title" | |||
onClick={titleOnClick} | |||
onClick={configuration.doubleClickMarkAsRead ? titleOnMultiClick : titleOnClick} |
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.
We would probably want to skip smartphones here to avoid the delay there.
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.
Agreed.
event.preventDefault(); | ||
}, | ||
1: titleOnClick, | ||
2: useCallback( |
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.
useCallback
would be needed if handler[2]
was used in dependencies list of useEffect
but in this case, the whole handler
object is used so that needs to be wrapped in useMemo
instead, see https://react.dev/reference/react/useMemo#memoizing-a-dependency-of-another-hook
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 probably miss a lot of React's basic rendering concept to truly understand that 🙈 So, please excuse if I don't understand it right away, but from your info and the docs, combined with my limited understanding, it should look like the following? 🤔
const titleOnMultiClick = useMultiClickHandler(useMemo(() => {
0: (event) => {
event.preventDefault();
},
1: (event) => handleToggleOpenClick({ event, history, location, expanded, id: item.id, target: '.entry-title' }),
2: (event) => {
if (canWrite && !selfoss.isSmartphone()) {
handleToggleReadClick({ event, unread: item.unread, id: item.id });
}
)
}, [ canWrite, history, location, expanded, item.unread, item.id ]));
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.
Yes, that looks correct to me.
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 do not like the delay. selfoss is already far too slow for my taste (still have some optimizations in the pipeline) so it cannot be the default.
The code is pretty nice and I can see how easier access to toggling read status without opening the entry would be convenient but I am still not sure double click is the right action for it. Usually double click action is used for opening e.g. file, contrasted with single click for selection. That also works without introducing delay because selection can be immediate.
Personally, I use auto_mark_as_read=1
which makes marking articles as read convenient. Though it will not help with unmarking. Maybe we could also consider other alternatives like point 3. from #969 (comment)
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.
True. How about dumping the delay?
It's just important to note that a double click then fires both events, i.e. a double click causes the entry to expand (or collapse) and marks it as read (or unread). It's a little less separation of concerns, but still quite convenient: You click once to expand the article, read it and close+mark it with a double click. If you decide not to read it yet (that's why I don't like auto_mark_as_read=1
: I often decide not to read an article straight away after expanding it, but later), you just collapse it with a single click.
The only disadvantage is that one can't mark an entry as read without opening it. I can think about multiple solutions for that: The first is to additionally implement #969 (comment). Another solution might be to distinguish where the double click happened: Double clicking on the title also expands/collapses the entry, but a double click anywhere else on the header just marks it as read/unread. What do you think about that?
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 am afraid that without delay, one probably could not use this with scroll_to_article_header=1
. But maybe that is fine.
Generally, I am not a fan of different parts of the same element behaving differently without some affordance to distinguish those areas.
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.
Yes, this isn't going to work with scroll_to_article_header=1
. So, shall we go with the delay-less solution? We can solve #969 (comment) another time.
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.
@jtojnar Can you please take a look into this? Just need a quick decision, I'll implement it accordingly then.
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.
Yeah, removing the delay sounds good. It should make the patch much shorter so there will be less of a chance of the new code path bitrotting, which is my primary concern.
### `double_click_mark_as_read` | ||
<div class="config-option"> | ||
|
||
set this to `1` to mark an item as read when double clicking on it. |
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.
set this to `1` to mark an item as read when double clicking on it. | |
Set this to `1` to enable toggling item’ (un)read state by double clicking the title. |
Ten years after #450 (I'm feeling old...) it's time to try again to finally bring feature to upstream: Double click on an entry's title to mark it as read (and double click again to mark it as unread; disabled on smartphones due to the different UI concept).
I believe that this should be the default behaviour, but since it must add a delay to distinguish between single and double clicks, some people might find it obstructive, thus I've deliberately added it as opt-in using the new
double_click_mark_as_read
option.This was the first time I worked with React. So please check whether the code I've written (including the
handleMultiClick
function and its usage, I'm not sure whetheruseCallback
still makes sense there) is like it's expected for React.