Update dark/light theme toggle #1154
Conversation
…tring is not empty, in ChallangeController and ChallengeUI
There was a problem hiding this comment.
Hi @Shubham-Patel07 , the icons look awesome! There are just a few things that will need some move love and then we are good to go, can you please:
- make sure the toggle only renders a switch when clicked/tapped
- make sure the attributes are back so the cypress tests can run (and/or adapt the cypress test for the toggle switching)
Cheers!
| <label> | ||
| <span class="visually-hidden">Dark</span> | ||
| <input type="radio" value="dark" name="theme"> | ||
| <span class="checkmark" th:attr="data-cy='dark-mode-radio'">🌜</span> |
There was a problem hiding this comment.
the attributes are required for people who have trouble seeing and are required for the cypress tests to run. Can you add the data-cy attrs to your new implementation please?
And the cypress tests need updating to use the new labels to be able to switch themes
| initialTheme = localStorage.getItem('darkMode') === 'true' | ||
| } else { | ||
| initialTheme = darkModeMediaQuery.matches | ||
| } |
There was a problem hiding this comment.
can we prevent the switch from "animating" every time we load a page in dark mode please? this makes the rendering time quite lengthy . I rather only have the animation when somethiing changes by the user.
This is also one of the reasons why the cypress tests are failing.
There was a problem hiding this comment.
i tried but i am not able to resolve that flickering issue every time we navigate to different page, but about the cypress test its failing cause they are written as respect to radio button logic and i have updated to checkbox hence they are failing. so i am tweaking the test script a bit
There was a problem hiding this comment.
the flickering issue: might that have to do with how you load the initial state and how you render it ;-)?
Happy to review again when you have the tests fixed and the attributes th:attr="data-cy='dark-mode-<widgettype>'" back again.
There was a problem hiding this comment.
cypress seems to work, i guess there are 2 things left:
- In the console of the browser, I now get an error: "Error: attribute d: Expected number, "…7.18492 13.0678 ..."."
- it seems that when we set it to "checked" in the dom it only animates when we want to go to light mode. @mikewoudenberg would you know what to do here?
src/main/java/org/owasp/wrongsecrets/challenges/ChallengeUI.java
Outdated
Show resolved
Hide resolved
src/main/java/org/owasp/wrongsecrets/challenges/ChallengesController.java
Outdated
Show resolved
Hide resolved
…box and now all test working properly and examining appropriately
|
I have pushed the test scripts please look into flickering issue |
|
I have looked into it, but could not find a solution. Will have to postpone that for a while unless @bendehaan finds a solution maybe. I am also not sure if this control is usable for the people with visibility issues. |
What kind of changes does this PR include?
Description
New enhancement done, Updated the radiio button based toggle switch to checkbox based toggle switch
from
updated one,