-
Notifications
You must be signed in to change notification settings - Fork 272
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
Preferences Reset Feature #917
Conversation
Hi @araujoarthur0 if you get a chance, could you provide some feedback about these changes? We'd appreciate any suggested edits or idea - thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
css/styles.css
Outdated
@@ -20,6 +20,7 @@ html[data-theme="light"] { | |||
--table-header-label-bground: var(--table-bground); | |||
--table-header-label-shadow: var(--table-border); | |||
--error: red; | |||
--reset-color: rgb(242, 93, 77); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the color doesn't change between themes, you can keep it in the element description below.
src/preferences.js
Outdated
{ | ||
preferences = defaultPreferences; | ||
applyTheme(defaultPreferences['theme']); | ||
$('.reset-button').text('Resetted!'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Past of the verb reset is simply "reset"
I don't think you need to change the text itself after clicking. If the button has visual feedback and the other fields are changing value immediately, the user will already consider the change as done. Or you can use a success dialog saying that all fields were reset.
src/preferences.js
Outdated
@@ -7,6 +7,32 @@ import { translatePage } from '../renderer/i18n-translator.js'; | |||
let usersStyles; | |||
let preferences; | |||
|
|||
const defaultPreferences = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These already have a default in the main process file for preferences. Can you instead obtain these from the process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time-to-leave/js/user-preferences.js
Line 30 in 74e128e
const defaultPreferences = { |
Hi @sisir-umich! Can you look into the issues Arthur and I brought up and update the PR, please? Thanks! |
Definitely - thanks for the suggestions! We do have our final exams this week and next week, so we plan to be working on these revisions after a few weeks. We'll let you know if we get stuck anywhere. |
@sisir-umich are you intending to finish this PR? Thanks! |
@tupaschoal I haven't gotten a chance to finish this PR, thanks. |
@sisir-umich Do you think you'll be able to return to this? |
I resumed this on #1055, sorry for closing instead of taking it in your repo's main. |
Related issue
Closes #891
Context / Background
It was recently requested that a feature to reset preferences to their default values would be useful, so this implements that functionality.
What change is being introduced by this PR?
This PR is to review a "Reset" button in the preferences window that returns the preferences to their default values. To indicate that the reset occurred, the button's text changes to "Resetted!" (Attached are screenshots of this functionality). Upon closing the Preferences window, the preferences return to default in the main TTL window. To accomplish this, I added a new button to the src/preferences.html file with styling in the css/styles.css file. The corresponding functionality is implemented in the resetPreferences() method in src/preferences.js. Since it was difficult to update the preferences within the window as soon as the button is pressed and to create a pop-up, the text change on the button can indicate to the user that their action was successful. However, this is a rudimentary solution, and changes can be made by modifying the resetPreferences() method.
How will this be tested?
There has been a unit test added that performs a click on the new button using Jquery, and the corresponding text change verifies that the button responds to the click. Underlying preferences changes can be verified by running the applications, changing some preferences, pressing "Reset", and seeing if the changes occur.