Proof-of-concept: theming in saka #103

fluffynuts · 2020-02-13T07:52:02Z

Type of Change

Put an [x] for the relevant option

Bugfix/Cleanup (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Summary Of Changes

Why is this change needed?

Saka is really useful, but people with accessibility requirements may struggle to use it. Theming can help:

inverted / darker colors
higher contrast
larger fonts

This PR is a proof-of-concept for theming in Saka. If the approach is acceptable, I'd like to make a "proper" dark theme, and perhaps a "large" theme, in separate pull requests. In addition, documentation probably needs to be updated -- again, I'll gladly do so if the approach to the feature itself is deemed acceptable.

Does this close any open issues?

Doesn't close, but does relate to #57

Checklist

Tests are passing locally
Updated the README/Wiki documentation (if relevant)

…s only loaded in configuration screens

pureooze

Thanks for making the PR, I'm definitely interested in adding this feature now. The PR looks good so far, just had some questions/comments. Keep up the good work!

src/options/Main/OptionsList/ThemeSelection.jsx

package.json

pureooze · 2020-02-14T22:38:37Z

src/options/Main/OptionsList/index.jsx

@@ -13,7 +14,8 @@ export default class OptionsList extends Component {
      isLoading: true,
      mode: 'tab',
      showEmptySearchSuggestions: true,
-      enableFuzzySearch: true
+      enableFuzzySearch: true,
+      theme: 'light'


Instead of using strings can we use something similar to enums? I think it would be sufficient to create an object that maps to the valid themes. Then we could just access them using Themes.Light. This will help prevent mistakes with typos 😉. I realize the modes don't do this but that should probably be refactored later.

ok, I'll have a look into it (:

pureooze · 2020-02-14T22:39:33Z

src/options/Main/OptionsList/index.jsx

@@ -30,7 +32,8 @@ export default class OptionsList extends Component {
        isLoading: false,
        mode: sakaSettings.mode,
        showEmptySearchSuggestions: sakaSettings.showEmptySearchSuggestions,
-        enableFuzzySearch: sakaSettings.enableFuzzySearch
+        enableFuzzySearch: sakaSettings.enableFuzzySearch,
+        theme: sakaSettings.theme || 'light'


Same here with the theme enum thing

pureooze · 2020-02-14T22:40:27Z

src/saka/Main/Components/GUIContainer/index.jsx

@@ -25,6 +27,15 @@ export default class GUIContainer extends Component {
    this.setState({ zoom });
  };

+  fetchTheme = async function fetchTheme() {
+    const storedSettings = ( await browser.storage.sync.get(['sakaSettings']) ) || {};
+    console.log(storedSettings);


Once we are ready to merge this should be deleted

again, was trying to figure stuff out at this point

I haven't written a Firefox addon before

I haven't dealt with (P)React before

when stuff wasn't working, I fell back on old-school debugging, only to find that the problem was that since I'd had to restart Firefox, it had unloaded my dev variant of the plugin and loaded back the original version 🤦‍♀️

pureooze · 2020-02-14T22:40:37Z

src/saka/Main/Components/GUIContainer/index.jsx

+    console.log(storedSettings);
+    const { sakaSettings } = storedSettings;
+    // fall back on light theme when no settings
+    const resolved = Object.assign({ theme: 'light' }, sakaSettings);


Also enum thing here too

pureooze · 2020-02-14T22:41:49Z

src/scss/themes/inverted.scss

@@ -0,0 +1,3 @@
+#GUIContainer[data-theme="inverted"] {
+  filter: invert(100%);


Is this actually how this should work? I'm not too familiar with this type of accessibility work so not sure if there is more that needs to be done? Thoughts?

This is a first-pass at a dark theme -- it works, but there are way better mechanisms for theming. Using data-theme='theme-name' is a good approach (well, it's an approach used quite often), but a better theme would have scss inside this outer selector to apply predetermined colors and fonts (for a larger-font variant).

The theming part is going to take some design -- this was the simplest dark theme I could come up with to test the process and see if you'd even be interested in a PR -- I didn't want to spend ages on polishing a theme only to have the PR either sit or just be plain rejected.

When the time comes to design proper themes, there should be at least (imo)

a beautiful dark theme that most people wanting a dark theme would use (like how Dark Reader makes the web look)

honestly, this is what I would use. The inverted theme makes the addon usable for me, but not particularly beautiful (: For me, a dark theme is more comfortable, but I can deal without. For some people, they can't see what's on the screen at all with a regular light theme, or they get light bleeding out of light areas over dark areas.

a high contrast dark theme for accessibility (with larger fonts)

a high contrast light theme for accessibility (with larger fonts)

for some people, light-on-dark is difficult to deal with -- they just need larger fonts and inputs

both of the latter should be tested on machines where default font sizes have been set larger, to see if those settings apply to the addon's overlay. I have a friend who can test for me there. I can also get feedback from him on accessible color choices -- I remember having a discussion with him quite a while ago about the accessibility themes in Windows and what did and didn't work there.

Sounds good, looking forward to seeing more!

fluffynuts · 2020-02-24T05:44:14Z

Apologies for no movement on this PR -- last week was a bit hectic and I kinda forgot about it on the weekend ): I'll get to it though (:

…oncept

codecov-io · 2020-03-22T07:27:04Z

Codecov Report

Merging #103 into master will increase coverage by 0.60%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   58.50%   59.11%   +0.60%     
==========================================
  Files          57       59       +2     
  Lines         711      724      +13     
  Branches       65       67       +2     
==========================================
+ Hits          416      428      +12     
- Misses        264      265       +1     
  Partials       31       31

Impacted Files	Coverage Δ
src/options/Main/OptionsList/index.jsx	`94.73% <50.00%> (-5.27%)`	⬇️
src/options/Main/OptionsList/ThemeSelection.jsx	`100.00% <100.00%> (ø)`
src/options/Main/OptionsList/themes.js	`100.00% <100.00%> (ø)`
src/saka/Main/Components/GUIContainer/index.jsx	`86.66% <100.00%> (+8.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9173048...f95fa5b. Read the comment docs.

♻️ enumerate themes for options list instead of hand-coding

fluffynuts · 2020-03-22T16:14:09Z

@pureooze I've tidied up the requested places and the inverted theme, applied to the options page and started on theming the core of the addon. However, I now find lots of places with hard-coded colors in style attributes (for example, Suggestion). Of course, this makes things harder to theme. I wanted to check if you were ok with me refactoring these bits of code to rather apply / remove css classes so that colors can come from theme providers?

pureooze · 2020-03-22T21:11:02Z

I wanted to check if you were ok with me refactoring these bits of code to rather apply / remove css classes so that colors can come from theme providers?

Yes that would be amazing 😃
Thanks for continuing to work on this!

fluffynuts · 2020-07-10T20:28:53Z

hi; honestly, I don't have time to do all of this, so I'm quitting. I'm not going to try fooling myself that I'll "get around to this at some point". As a proof-of-concept, I think this PR had legs. If it had been accepted as a reasonable MVP, I think I might have had motivation to continue. Honestly, right now, I don't. I have too many other things I'd like to make. So I guess this is the end of the road. It was fun.

fluffynuts added 3 commits February 13, 2020 09:45

🎨 proof-of-concept for theming with initial inverted theme

028b188

✅ fix tests failing because no settings are stored

a3cf9df

🐛 should perform theme fetch at componentWillMount, otherwise theme i…

b2f58d5

…s only loaded in configuration screens

pureooze reviewed Feb 14, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into themes-proof-of-c…

62adad8

…oncept

fluffynuts added 5 commits March 22, 2020 09:35

👌 fix formatting, remove unnecessary imports

883c082

👌 remove dev dep

bf0e0a4

👌 introduce enum-like structure for theme names

f95fa5b

♻️ enumerate themes for options list instead of hand-coding

🎉 inverted theme applies properly to options and main ui

93d1d0b

🎉 some progress on a dark theme

156a014

🚧

c5f945f

fluffynuts closed this Jul 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof-of-concept: theming in saka #103

Proof-of-concept: theming in saka #103

fluffynuts commented Feb 13, 2020

pureooze left a comment

pureooze Feb 14, 2020

fluffynuts Feb 16, 2020

pureooze Feb 14, 2020

pureooze Feb 14, 2020

fluffynuts Feb 16, 2020

pureooze Feb 14, 2020

pureooze Feb 14, 2020

pureooze Feb 14, 2020

fluffynuts Feb 16, 2020

fluffynuts Feb 16, 2020 •

edited

Loading

pureooze Feb 17, 2020

fluffynuts commented Feb 24, 2020

codecov-io commented Mar 22, 2020 •

edited

Loading

fluffynuts commented Mar 22, 2020

pureooze commented Mar 22, 2020 •

edited

Loading

fluffynuts commented Jul 10, 2020

		@@ -0,0 +1,3 @@
		#GUIContainer[data-theme="inverted"] {
		filter: invert(100%);

Proof-of-concept: theming in saka #103

Proof-of-concept: theming in saka #103

Conversation

fluffynuts commented Feb 13, 2020

Type of Change

Summary Of Changes

Why is this change needed?

Does this close any open issues?

Checklist

pureooze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluffynuts Feb 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluffynuts commented Feb 24, 2020

codecov-io commented Mar 22, 2020 • edited Loading

Codecov Report

fluffynuts commented Mar 22, 2020

pureooze commented Mar 22, 2020 • edited Loading

fluffynuts commented Jul 10, 2020

fluffynuts Feb 16, 2020 •

edited

Loading

codecov-io commented Mar 22, 2020 •

edited

Loading

pureooze commented Mar 22, 2020 •

edited

Loading