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): grey out read articles even if starred #547

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

MauroGuida
Copy link
Contributor

Hi, in this pull request I have added a new Flow Option. When the Always highlight starred articles option is unchecked all unread articles will be displayed with 1f alpha, while all read articles, including the starred ones, will be displayed with 0.5f alpha.

This option does not impact the content displayed in any category; rather, it only influences the alpha blending of read articles.

The default behavior remains unchanged (this new option is enabled by default). Therefore, everything behaves as usual until the user manually unchecks the Always highlight starred articles option in Settings -> Color & Style -> Flow Page.

Is this useful?
I have many feeds on my FreshRSS server, and I often star some of them so I can read them later. Without this option, it's harder to distinguish between what I have already read and what I have saved to read later.

The default behavior is fine for most of us, but I believe that this new option adds more customization for the end user without interfering with app development and maintenance.

@Ashinch Ashinch self-requested a review January 19, 2024 12:07
@Ashinch
Copy link
Owner

Ashinch commented Jan 19, 2024

Thank you, I will take some time to review it

@JunkFood02
Copy link
Collaborator

Could you please provide some screenshots of your implementation?

@MauroGuida
Copy link
Contributor Author

Could you please provide some screenshots of your implementation?

Yes sure!

Always highlight starred articles Enabled (Default):
ON1
ON2

Always highlight starred articles Disabled:
OFF1
OFF2

@nvllz
Copy link
Contributor

nvllz commented Jan 20, 2024

Am I understanding this correctly? This switch, when turned off, makes read starred articles appear as regular read articles (grayed out, 0.5f)?

If so, I think it would be more natural to have the setting work the other way around. Let's call it "Dim starred articles on read", which when turned on (assuning it's meant to be it's off by default) makes the read starred articles 0.5f. Because the "Always highlight starred articles" doesn't tell what's it really about.

@Ashinch
Copy link
Owner

Ashinch commented Jan 20, 2024

Am I understanding this correctly? This switch, when turned off, makes read starred articles appear as regular read articles (grayed out, 0.5f)?

If so, I think it would be more natural to have the setting work the other way around. Let's call it "Dim starred articles on read", which when turned on (assuning it's meant to be it's off by default) makes the read starred articles 0.5f. Because the "Always highlight starred articles" doesn't tell what's it really about.

The read articles are styled 0.5f, unread articles are styled 1f, and starred articles are styled 1f (it's special, that way starred articles can't tell if they're read or not, which is the design style in Reeder). The PR implements a switch option that gives users the freedom to choose whether or not they want starred articles to visually tell if they've been read or not, just like regular articles.

ReadYou emulates the design style of Reeder, so starred items are highlighted just like in Reeder.

ReadYou also embraces diverse styles, much like Material You, so this switch option makes sense.

image

@Ashinch Ashinch added this to the 0.9.12 milestone Jan 20, 2024
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.

app/src/main/java/me/ash/reader/ui/page/settings/color/flow/FlowPageStylePage.kt: 196~205

Hi, @MauroGuida

Can you move the position of the new option to the previous one of TonalElevation?

@nvllz
Copy link
Contributor

nvllz commented Jan 21, 2024

The read articles are styled 0.5f, unread articles are styled 1f, and starred articles are styled 1f (it's special, that way starred articles can't tell if they're read or not, which is the design style in Reeder). The PR implements a switch option that gives users the freedom to choose whether or not they want starred articles to visually tell if they've been read or not, just like regular articles.

Sure, but I don't think most users will understand what this "always highlight starred articles" change means. For us it's clear, it's meant to mark read (apply 0.5f) starred articles (now starred are always displayed as 1f). But if I never came here, the phrase "always highlight starred articles" would mean for me to put a frame over all articles I have starred, and I wouldn't see any real effect of toggling this option one way or the other at a glance.

This setting is about dimming starred articles when they are read (using 0.5f, as now they are always 1f regardless of the read/not read state).

It's my idea to make this option more user friendly:

  1. Instead of making it on by default, make it off and change the logic (off = always 1f as it is now, on = apply 0.5f when opening starred articles).
  2. Name it properly, so people don't get confused about the lack of highlighting (it's not really highlighting anything, because starred articles still look like regular ones unless read, I personally wouldn't notice that's the purpose of this setting) - my idea is to name it "Dim starred articles on read".

I really want this option in the app, I'd find it very useful and use it. But like I said - if I never came here to check this PR, I'd never know what this switch in the settings is about. Please reconsider my idea.

@Ashinch
Copy link
Owner

Ashinch commented Jan 22, 2024

@nvllz @MauroGuida

Instead of making it on by default, make it off and change the logic (off = always 1f as it is now, on = apply 0.5f when opening starred articles).

Yes, It should be turned off by default.

Name it properly, so people don't get confused about the lack of highlighting (it's not really highlighting anything, because starred articles still look like regular ones unless read, I personally wouldn't notice that's the purpose of this setting) - my idea is to name it "Dim starred articles on read".

Perhaps 'Dim starred articles on read' is a bit lengthy? It might not be very translation-friendly for some multi-character languages. Do we have other terms or names for it?

@MauroGuida
Copy link
Contributor Author

@nvllz @MauroGuida

Instead of making it on by default, make it off and change the logic (off = always 1f as it is now, on = apply 0.5f when opening starred articles).

Yes, It should be turned off by default.

Name it properly, so people don't get confused about the lack of highlighting (it's not really highlighting anything, because starred articles still look like regular ones unless read, I personally wouldn't notice that's the purpose of this setting) - my idea is to name it "Dim starred articles on read".

Perhaps 'Dim starred articles on read' is a bit lengthy? It might not be very translation-friendly for some multi-character languages. Do we have other terms or names for it?

You're correct; for instance, "Dim" in Italian translates to "Affiocare: Rendere fioco", which isn't ideal for the translation: "Affioca articoli preferiti letti", 'Affiocare' is not a very common verb in everyday usage, and using 'Affiocare' for text could be misleading.

I can't think of a better phrase than Dim starred articles on read that is equally concise and meaningful. A potential solution could be to use Dim starred articles on read and allow the translators to come up with an alternative wording, not necessarily a literal translation, that conveys the same meaning.

@JunkFood02
Copy link
Collaborator

Here's my take:

image

@MauroGuida
Copy link
Contributor Author

MauroGuida commented Jan 22, 2024

@JunkFood02

It's a great idea, personally I don't see why anyone would choose none, but the choice is up to the user and I really like it.

@Ashinch
Copy link
Owner

Ashinch commented Jan 23, 2024

Here's my take:

image

@JunkFood02 Nice idea, this is in line with the user's intuition.

It's a great idea, personally I don't see why anyone would choose none

I think @MauroGuida is right. We need gray to distinguish between read and unread items, so the option should be:

  • Read, excluding starred
  • Read

@nvllz
Copy link
Contributor

nvllz commented Jan 23, 2024

Good idea. For clarity consider naming the latter "All Read".

@JunkFood02 JunkFood02 self-assigned this Jan 23, 2024
@JunkFood02
Copy link
Collaborator

Sorry for force pushing commits, github is too hard for me 🥲

@JunkFood02 JunkFood02 changed the title Always highlight starred articles feat(ui): grey out read articles even if starred Jan 28, 2024
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.

Can you move the position of the new option to the previous one of TonalElevation?

@Ashinch Ashinch merged commit 7400cdf into Ashinch:main Jan 30, 2024
1 check passed
Ashinch added a commit that referenced this pull request Feb 6, 2024
* build(deps): bump up dependencies, compile sdk, and gradle version (#502)

* build(deps): bump up dependencies, compile sdk, and gradle version

* build(deps): remove redundant safe-args plugin

* build(deps): update Compose BOM to `2024.01.00` & compiler to `1.5.8`

* fix(i18n): configuration loss when switching locale (#541)

* fix(i18n): configuration loss when switching locale

* feat(locale): enable auto-localeconfig

* feat(i18n): add languages to in-app language picker (#571)

* feat(i18n): add languages to in-app language picker

* fix(i18n): locale system settings not working for Android 13

* feat(i18n): show selected language at settings page

* fix(ci): ignore ExtraTranslation for linter

* feat(i18n): add fallback in in-app language picker for A13+

* chore: clean up

* fix(ui): ProgressIndicator crashes in m3 1.1.2

* fix(ui): NavigationBarItem color

* feat(ui): grey out read articles even if starred (#547)

* refactor(ui): improve add account dialog

* fix(ui): accessing listState on io thread causes app to crash

* fix(ui): NavigationBar text color

* feat(ui): show full screen image viewer when clicking on images (#578)

* feat(ui): add crash report activity to handle uncaught exceptions (#576)

* feat(ui): swipe up and down to switch between articles (WIP)

* feat(ui): update animation

* docs(ui): add comments on pull to load implementation

* feat(ui): move the indicator to another file

* build: revert changes

* feat(ui): make the transition directions match the content changes

---------

Co-authored-by: MauroGuida <[email protected]>
Co-authored-by: Ash <[email protected]>
Co-authored-by: Ash <[email protected]>
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.

None yet

4 participants