-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,30 @@ function setupLightbox({ | |
})); | ||
} | ||
|
||
function useMultiClickHandler(handler, delay = 400) { | ||
const [state, setState] = useState({ clicks: 0, args: [] }); | ||
|
||
useEffect(() => { | ||
const timer = setTimeout(() => { | ||
setState({ clicks: 0, args: [] }); | ||
|
||
if (state.clicks > 0 && typeof handler[state.clicks] === 'function') { | ||
handler[state.clicks](...state.args); | ||
} | ||
}, delay); | ||
|
||
return () => clearTimeout(timer); | ||
}, [handler, delay, state.clicks, state.args]); | ||
|
||
return (...args) => { | ||
setState((prevState) => ({ clicks: prevState.clicks + 1, args })); | ||
|
||
if (typeof handler[0] === 'function') { | ||
handler[0](...args); | ||
} | ||
}; | ||
} | ||
|
||
function stopPropagation(event) { | ||
event.stopPropagation(); | ||
} | ||
|
@@ -104,7 +128,7 @@ function closeFullScreen({ event, history, location, entryId }) { | |
} | ||
|
||
// show/hide entry | ||
function handleClick({ event, history, location, expanded, id, target }) { | ||
function handleToggleOpenClick({ event, history, location, expanded, id, target }) { | ||
const expected = selfoss.isMobile() ? '.entry' : '.entry-title'; | ||
if (target !== expected) { | ||
return; | ||
|
@@ -123,6 +147,14 @@ function handleClick({ event, history, location, expanded, id, target }) { | |
} | ||
} | ||
|
||
// mark entry read/unread | ||
function handleToggleReadClick({ event, unread, id }) { | ||
event.preventDefault(); | ||
event.stopPropagation(); | ||
|
||
selfoss.entriesPage.markEntryRead(id, unread == 1); | ||
} | ||
|
||
// load images | ||
function loadImages({ event, setImagesLoaded, contentBlock }) { | ||
event.preventDefault(); | ||
|
@@ -257,6 +289,8 @@ export default function Item({ currentTime, item, selected, expanded, setNavExpa | |
[currentTime, item.datetime] | ||
); | ||
|
||
const canWrite = useAllowedToWrite(); | ||
|
||
const previouslyExpanded = usePreviousImmediate(expanded); | ||
const configuration = useContext(ConfigurationContext); | ||
|
||
|
@@ -360,15 +394,30 @@ export default function Item({ currentTime, item, selected, expanded, setNavExpa | |
}, [configuration, expanded, item.id, item.unread, previouslyExpanded]); | ||
|
||
const entryOnClick = useCallback( | ||
(event) => handleClick({ event, history, location, expanded, id: item.id, target: '.entry' }), | ||
(event) => handleToggleOpenClick({ event, history, location, expanded, id: item.id, target: '.entry' }), | ||
[history, location, expanded, item.id] | ||
); | ||
|
||
const titleOnClick = useCallback( | ||
(event) => handleClick({ event, history, location, expanded, id: item.id, target: '.entry-title' }), | ||
(event) => handleToggleOpenClick({ event, history, location, expanded, id: item.id, target: '.entry-title' }), | ||
[history, location, expanded, item.id] | ||
); | ||
|
||
const titleOnMultiClick = useMultiClickHandler({ | ||
0: (event) => { | ||
event.preventDefault(); | ||
}, | ||
1: titleOnClick, | ||
2: useCallback( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that looks correct to me. |
||
(event) => { | ||
if (canWrite && !selfoss.isSmartphone()) { | ||
handleToggleReadClick({ event, unread: item.unread, id: item.id }); | ||
} | ||
}, | ||
[canWrite, item.unread, item.id] | ||
) | ||
}); | ||
|
||
const starOnClick = useCallback( | ||
(event) => { | ||
event.preventDefault(); | ||
|
@@ -379,12 +428,8 @@ export default function Item({ currentTime, item, selected, expanded, setNavExpa | |
); | ||
|
||
const markReadOnClick = useCallback( | ||
(event) => { | ||
event.preventDefault(); | ||
event.stopPropagation(); | ||
selfoss.entriesPage.markEntryRead(item.id, item.unread == 1); | ||
}, | ||
[item] | ||
(event) => handleToggleReadClick({ event, unread: item.unread, id: item.id }), | ||
[item.unread, item.id] | ||
); | ||
|
||
const loadImagesOnClick = useCallback( | ||
|
@@ -411,8 +456,6 @@ export default function Item({ currentTime, item, selected, expanded, setNavExpa | |
[item.source] | ||
); | ||
|
||
const canWrite = useAllowedToWrite(); | ||
|
||
const _ = useContext(LocalizationContext); | ||
|
||
const sharers = useSharers({ configuration, _ }); | ||
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. |
||
> | ||
<span | ||
className="entry-title-link" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I know, I usually do double click to select words. |
||
} | ||
|
||
.entry-title a { | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -277,6 +277,12 @@ Hide read articles on mobile devices. | |||||
Set to `0` to stop the interface from scrolling to the article header when an article is opened. | ||||||
</div> | ||||||
|
||||||
### `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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
</div> | ||||||
|
||||||
### `env_prefix` | ||||||
<div class="config-option"> | ||||||
|
||||||
|
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.