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

Change switch color depending on state #1182

Closed
wants to merge 1 commit into from

Conversation

RumovZ
Copy link
Collaborator

@RumovZ RumovZ commented May 20, 2021

On the forum, I said the existing green wouldn't work in day mode. After a bit of experimenting and changing the blue to a lighter hue as well, I take that back.
@dae, we had the colour discussion before. Do you think this is an improvement? @kleinerpirat, what do you think? The switch is also a bit less obtrusive with the new colours.

grafik grafik
grafik grafik

@kleinerpirat
Copy link
Contributor

kleinerpirat commented May 20, 2021

Do you think this is an improvement? @kleinerpirat, what do you think?

I think this PR is a great improvement!


Copying bits from the forum post I just made here:

2 variants on Codepen to experiment

  • Variant 1: Instead of the knob color, the background-color of the switch changes.
    • advantage: better readability of the letter
  • Variant 2: The status quo of this PR, works just as well as a visual clue.
    • advantage: more subtle, no further change required

card viewnote view


Update: In the forum I suggested two different variants and the poll suggests that the colored knob (variant 2 in my codepen) is preferred - and it's also less work for you!

@RumovZ
Copy link
Collaborator Author

RumovZ commented May 21, 2021

Thanks for the demo. I think that you can only really tell if it works if it's embedded in the actual GUI, though.

@kleinerpirat
Copy link
Contributor

Sure, you're right! I didn't get around to building Anki from source yet, but since there are some UX-Design challenges ahead where I could potentially help, I guess it's time that I do :)

@dae
Copy link
Member

dae commented May 21, 2021

The letter does feel a bit difficult to read at the moment. I have no strong feelings about using a separate colour for notes mode if that's what people would prefer :-) Would be interesting to see how @kleinerpirat's variants look like in the program too

@RumovZ
Copy link
Collaborator Author

RumovZ commented May 21, 2021

grafik grafik
grafik grafik

Of course, the contrast can be increased independent of the variant by using the foreground colours as @kleinerpirat has done in the meantime. I personally prefer the unobtrusive low-contrast version. Now that the mode can be identified by position, label and colour, legibility should be less important.

@RumovZ
Copy link
Collaborator Author

RumovZ commented May 21, 2021

The narrow bright area in night mode looks a bit dirty in my opinion, but removing the margin doesn't work in Qt (and arguably in svelte neither):
grafik

So I would fo for a different style in that case:
grafik

@kleinerpirat
Copy link
Contributor

I've experimented a bit with that look as well. Here I gave the knob the same radius as the path:

Night Mode

image image

Day Mode

image image

I'm getting really switchy vibes from this design. It's quite satisfying to toggle.

  • knob color: --frame-bg
  • label color: --text-fg
  • width of path reduced by 6.25% (really arbitrary, but I found the original a bit too wide)

To get rid of the narrow bright area, I changed this:

anki/qt/aqt/switch.py

Lines 37 to 40 in 25685f7

self._path_radius = radius
self._knob_radius = radius - self._margin
self._left_position = self._position = self._path_radius + self._margin
self._right_position = 3 * self._path_radius + self._margin

to this:

        self._path_radius = radius
        self._knob_radius = radius
        self._left_position = self._position = self._path_radius
        self._right_position = 2.8125 * self._path_radius + self._margin

(That 2.8125 is meant to correct for my 6.25% path-width-reduction.)

And this is the knob_rectangle:

    def _current_knob_rectangle(self) -> QRectF:
        return QRectF(
            self.position - self._knob_radius,  # type: ignore
            self._margin,
            2 * self._knob_radius,
            2 * self._knob_radius,
        )

@dae
Copy link
Member

dae commented May 22, 2021

  • I've just tried out the original PR change, and it's already looking pretty good - night mode is easy to read, and the blue C seems fairly easy to read in day mode (green is a little harder)
  • Increasing the circle size to remove the tiny margins does seem like an improvement
  • I had trouble replicating the changes in your code @kleinerpirat - I'd recommend you use 'git diff > file.txt' and then copy&paste the text here, so we can apply the patch directly.

@kleinerpirat
Copy link
Contributor

kleinerpirat commented May 22, 2021

Sure, here's the diff:

diff --git a/qt/aqt/switch.py b/qt/aqt/switch.py
index b478fbf4e..d8b739741 100644
--- a/qt/aqt/switch.py
+++ b/qt/aqt/switch.py
@@ -35,9 +35,9 @@ class Switch(QAbstractButton):
         self._left_color = left_color
         self._right_color = right_color
         self._path_radius = radius
-        self._knob_radius = radius - self._margin
-        self._left_position = self._position = self._path_radius + self._margin
-        self._right_position = 3 * self._path_radius + self._margin
+        self._knob_radius = radius
+        self._left_position = self._position = self._path_radius
+        self._right_position = 2.85 * self._path_radius + self._margin
 
     @pyqtProperty(int)  # type: ignore
     def position(self) -> int:
@@ -61,13 +61,13 @@ class Switch(QAbstractButton):
         return self._right_label if self.isChecked() else self._left_label
 
     @property
-    def knob_color(self) -> QColor:
+    def path_color(self) -> QColor:
         color = self._right_color if self.isChecked() else self._left_color
         return theme_manager.qcolor(color)
 
     def sizeHint(self) -> QSize:
         return QSize(
-            4 * self._path_radius + 2 * self._margin,
+            3.8 * self._path_radius + 2 * self._margin,
             2 * self._path_radius + 2 * self._margin,
         )
 
@@ -85,7 +85,7 @@ class Switch(QAbstractButton):
         self._paint_label(painter)
 
     def _paint_path(self, painter: QPainter) -> None:
-        painter.setBrush(QBrush(theme_manager.qcolor(colors.FRAME_BG)))
+        painter.setBrush(QBrush(self.path_color))
         rectangle = QRectF(
             self._margin,
             self._margin,
@@ -97,17 +97,18 @@ class Switch(QAbstractButton):
     def _current_knob_rectangle(self) -> QRectF:
         return QRectF(
             self.position - self._knob_radius,  # type: ignore
-            2 * self._margin,
+            self._margin,
             2 * self._knob_radius,
             2 * self._knob_radius,
         )
 
     def _paint_knob(self, painter: QPainter) -> None:
-        painter.setBrush(QBrush(self.knob_color))
+        painter.setBrush(QBrush(theme_manager.qcolor(colors.FRAME_BG)))
+        painter.setPen(QColor(theme_manager.qcolor(colors.FAINT_BORDER)))
         painter.drawEllipse(self._current_knob_rectangle())
 
     def _paint_label(self, painter: QPainter) -> None:
-        painter.setPen(QColor("white"))
+        painter.setPen(QColor(theme_manager.qcolor(colors.TEXT_FG)))
         font = painter.font()
         font.setPixelSize(int(1.5 * self._knob_radius))
         painter.setFont(font)

This is with a 5% width decrease and I also gave the knob a faint border (gives it a bit more pop against the background). The only thing that still bugs me here is that the label seems to be a bit off-center (lower). Perhaps @RumovZ knows how to fix this.

@RumovZ
Copy link
Collaborator Author

RumovZ commented May 22, 2021

I feel that the knob should either be smaller or larger than the path. That's personal preference, of course, but it's also practical to avoid the ugly outline seen in my screenshot above.
Changing the constants to arbitrary values isn't a viable solution, because it makes figuring out the exact positions intractable. And without comprehensible positioning, I'm not confident that it will work with different parameters and environments.
As for the label, I think that's just caused by the size being too large.
I'll make a new PR with the other style I mentioned.

@dae
Copy link
Member

dae commented May 24, 2021

Thank you both, I've just merged in #1188

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

3 participants