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

feat(ui): long press on an item to show context menu #613

Merged
merged 12 commits into from
Mar 10, 2024
Merged

Conversation

JunkFood02
Copy link
Collaborator

@JunkFood02 JunkFood02 commented Feb 12, 2024

A workaround or add-on before we implement #585
Close #611

To Do:

  • Mark above/below as read
  • Enter/exit transition for dropdown menu (optional)

@JunkFood02 JunkFood02 added this to the 0.9.13 milestone Feb 12, 2024
@JunkFood02 JunkFood02 marked this pull request as ready for review February 13, 2024 08:06
@nvllz
Copy link
Contributor

nvllz commented Feb 13, 2024

Just tested, looks stable. I'd change a few things:

  • highlight the article that opened the context menu*.
  • change the background color of the context menu to match the color of the highlighted article or some other accent color, as the current one is rather bland**.

But... maybe make it a bit easier? Place read/star/share in a grid, as these actions should be more familiar to users, and have the mark above/below as read as it is now. Something like the context menu in Chrome:

5f59f953-dc2b-4ebd-a3e2-c0db7d64b4e0

Note the divider style, which could also be applied (less visible, more subtle). This way the menu would be much more handy.

* By highlighting the item card, I mean the state when you hold your finger on it, which triggers the menu. But now, when you stop holding it, the bg color disappears - my idea is to make this display until the context menu is closed.

9d98fe0d-8c90-44da-8490-368671f3b113.mp4

(Recorded it yesterday with an older build, but still applies)

** Could be the same as the triggered article tint

I also find that this shadow border does not match the overall design of the app. Maybe make the background color of the context menu the same as the background color of the app, and give the context menu a subtle outline with a tint of the color of the highlighted item? Hmm... I imagine that would be a more appealing way to handle it.

Great work as always!

@JunkFood02
Copy link
Collaborator Author

By highlighting the item card, I mean the state when you hold your finger on it, which triggers the menu. But now, when you stop holding it, the bg color disappears - my idea is to make this display until the context menu is closed.

It's a bug and I just fixed it

highlight the article that opened the context menu*.

Not sure about whether I have the energy to make another design for it, but I'll give it a try when I have time.

But... maybe make it a bit easier? Place read/star/share in a grid

No, just follow the guidelines

shadows are ugly

Yes, blame google

@JunkFood02 JunkFood02 marked this pull request as draft February 15, 2024 05:35
@JunkFood02 JunkFood02 marked this pull request as ready for review March 6, 2024 05:38
@JunkFood02
Copy link
Collaborator Author

JunkFood02 commented Mar 6, 2024

Mark above/below as read

After evaluating the existing code base and the current model of how we handle read status updating, I find it difficult to implement new logic to make the markAsRead method(s) accept an extra after: Date.

Problems

  • APIs are all written to mark as read before a certain date, modifying such APIs needs sigfinicant refactoring to the current Repositories, DAOs, and even remote API services
  • SQL queries are being used heavily for filtering, searching, and presenting the paged data to the UI layer. Writing new SQL queries, or adding new fields on existing queries is painful.

Proposed Solution

In this PR, two new methods are added to the code base, updateReadStatusByIdSet in AbstractRssRepository, and markAsReadFromListByDate in FlowViewModel.

  • updateReadStatusByIdSet

Just what its name suggests, bulk updating the read status by a Set of article id

  • markAsReadFromListByDate

Filter the list before or after the given Date, map it to the set of article id, then call updateReadStatusByIdSet

Considerations

  • Ease of implementation: no structural changes made to code base, ~60 LoC
  • Consistent with UI: Users are only presented with limited data, operations on the same paging data list won't bring unexpected behaviors, whether it's being filtered or/and being searched
  • The numbers of articles to update: "Mark above as read" should only update limited articles when a user is scrolling down the flow list. "Mark below as read" doesn't seem practical but preserved for consistency

To-Do

  • Bulk updating for FeverAPI (updateReadStatusByIdSet) cc @Ashinch

@JunkFood02
Copy link
Collaborator Author

It seems like that fever doesn't support bulk update of the read mark of multiple items, should we disable this feature on Fever accounts to avoid sync errors? @Ashinch

@Ashinch
Copy link
Owner

Ashinch commented Mar 8, 2024

It seems like that fever doesn't support bulk update of the read mark of multiple items, should we disable this feature on Fever accounts to avoid sync errors? @Ashinch

We can mark them as read in turn on the background threads. Can leave this todo to me.

@Ashinch
Copy link
Owner

Ashinch commented Mar 8, 2024

It seems like that fever doesn't support bulk update of the read mark of multiple items, should we disable this feature on Fever accounts to avoid sync errors? @Ashinch

Hi, @JunkFood02

#640 has been merged.

Usage: rssService.get().batchMarkAsRead(articleIds, false)

@JunkFood02 JunkFood02 requested a review from Ashinch March 8, 2024 11:09
Copy link
Owner

@Ashinch Ashinch left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long press an item in article list to show context menu with actions
3 participants