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

Remove font-weight change for High Contrast theme #775

Closed
wants to merge 2 commits into from

Conversation

tenkabuto
Copy link
Contributor

@tenkabuto tenkabuto commented Mar 15, 2019

The font-weight change in the High Contrast theme seems too aggressive to me. I suggest that the changes to background color and color are sufficient for providing the High Contrast.

I've built the app with this edit. Click here for a screenshot of it being used.

Comment out the font-weight change upon becoming high contrast
@tcitworld
Copy link
Member

Thanks! Could you remove the line instead of commenting it ?

@tenkabuto
Copy link
Contributor Author

@tcitworld Yes, I'll do that soon. 👍

Removed font-weight; previously I had just commented it out.
@tenkabuto
Copy link
Contributor Author

@tcitworld I made the changes! 🎆

@Strubbl
Copy link
Contributor

Strubbl commented Mar 15, 2019

How is the text looking with a font-weight: 500;? (400 is the default normal font weight) Is there any difference to normal?
I think for a contrast theme a slight weighter font might make sense.

@tenkabuto
Copy link
Contributor Author

@Strubbl Good idea. I didn't consider that. Here's a comparison photo for 400, 500, and 600. I think the 500 does indeed look better for a high contrast theme.

@Strubbl
Copy link
Contributor

Strubbl commented Mar 15, 2019

Thanks for the comparison. What do you think about the 500 variant?

I am not a user of this theme. If i was, i would prefer that variant for the better contrast compared to 400.

@Strubbl
Copy link
Contributor

Strubbl commented Mar 15, 2019

Sorry, overread your opinion on the 500.
So, let's go for that?

@di72nn
Copy link
Member

di72nn commented Mar 15, 2019

I believe the font weight was added for a reason: #84 - it probably makes some difference on e-ink screens. And if it does, maybe the font weight should be made optional somehow.

@shtrom any opinion on this?

@shtrom
Copy link
Contributor

shtrom commented Mar 16, 2019

Hum. I didn't remember even doing this. My nook is long gone, so I cannot test. I don't have much of an opinion either way. Feel free to merge!

@di72nn
Copy link
Member

di72nn commented Mar 19, 2019

Ok, thanks. I guess I'll involve people from #168 just in case.

@gerroon, @grote, @silberzwiebel, can you guys provide any insight into e-ink usage?

If nobody cares about the font weight, then it indeed makes sense to remove that to improve consistency between themes.

@tcitworld
Copy link
Member

Isn't the black theme also used on regular Android devices as an "energy saving" theme for AMOLED devices ?

@di72nn
Copy link
Member

di72nn commented Mar 19, 2019

Well, yeah. With dark themes the situation seems to be clear: the regular dark theme maybe used by everyone, the contrast dark may be especially useful for AMOLED, but the theme is as suitable for anyone else who likes pure black.

With the contrast light theme it may be a bit different: it may be used by people having devices with e-ink screens (but we don't usually hear about it unless there are some problems). So if the theme is used, it would be reasonable to copy the theme as "e-ink oriented" and remove the font weight from the regular contrast theme. Or something like that.

I'm just worried that there may be people whose experience may be worsened, but there's no telling whether they would be bothered to provide feedback. I probably overthink it though.

@tenkabuto
Copy link
Contributor Author

I agree that an alternate theme should be offered that includes increased fontweight, or eventually provide instead the option of increasing fontweight across themes.

I've been holding off on replying to this issue again until I've been able to try the app with the reduced fontweight. I only have the no-fontweight theme installed right now and I like it, but I'd like to test the mid-range one, too. 😅

@silberzwiebel
Copy link
Contributor

Thanks for pinging me! This is how I realized that changing the theme (from default to high contrast) actually gives a better reading experience on my e-ink device.

I had thoughts around "this font is too grayish" before but it never bothered me too much. I stick to high contrast on my device now, not sure what font-weight this means, I'm running wallabag app version 2.1.0 (109).

@tenkabuto
Copy link
Contributor Author

I have used the 500 font-weight, now, and it still feels too heavy to me while using San Serif typefaces.

With Serif typefaces enabled, I don't notice a significant difference --- the key difference here, between regular (non-high) contrast and high contrast when Serif is enabled, is the background color and text color, which appear to be too light with the regular contrast setting.

@di72nn
Copy link
Member

di72nn commented Mar 29, 2019

I suggest splitting current high contrast theme into two others: a high contrast theme without font-weight and another one specifically for e-ink screens. I can provide technical advice if needed.

@silberzwiebel if you feel like that should be the default theme on devices like yours, it is possible to set it as default here:

Themes.Theme theme = android.os.Build.MODEL.equals("NOOK")

@silberzwiebel
Copy link
Contributor

@di72nn Thanks for the pointer. However, since the manufacturer of my e-ink device seems to consider security a not-so-important issue, the android version is still stuck on 4.2. So, newer versions of wallabag won't run on that device (as android 5.0 is now the minimum required version), rendering an update of the default theme useless.

@Strubbl
Copy link
Contributor

Strubbl commented Apr 23, 2019

@silberzwiebel you can find a legacy release "app-release-legacy.apk" attached to the release page https://github.com/wallabag/android-app/releases/tag/2.2.0

Can you install APKs directly to the e-ink device?

@silberzwiebel
Copy link
Contributor

Yes, I know that, thanks! I was just pointing out that it doesn't make sense to add a new default theme for this specific device to the codebase, because the next release (with that codebase) won't run on the device anyway.

@tcitworld
Copy link
Member

Closed in favour of #782

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.

6 participants