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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Button to reset configuration #141

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@ericcornelissen
Copy link

ericcornelissen commented Jul 24, 2018

Adds a new button to the main menu that can be used to reset the configuration of SVGO. As requested in #101 (and simple-icons/#771).

Design considerations

  • There is currently no "Are you sure" popup when clicking the reset configuration button, might be an idea to add it (added with dfb7392)
  • The button is currently in the mainMenuUI, could also be placed in the settingsUI. I'm fine either way 馃槃

Clarification of the code

  • main-controller.js:277 was added because the reset button calls _compressSvg, but may be called when no SVG has been selected yet.
  • I'm emitting an event in main-menu.js:97 mainly because I couldn't think of a better approach

Tested on Desktop

  • Chrome
  • Firefox
  • Safari
  • IE/Edge

Tested on Mobile

  • Chrome
  • Firefox

ericcornelissen added some commits Jul 24, 2018

Add button to main menu to reset the configuration
By simply removing 'settings' key from the indexedDB 'keyval' 
ObjectStore.

As requested in #101
Reset the UI with the IndexedDB storage when resetting the config
Moved the `storage.delete` statement to the main-controller so the 
settingsUI can be updated easily. The main-controller is informed about 
a reset by the `'reset-config'` event from the mainMenuUI component.
Always remove existing configuration on reset
This allows for backwards compatability (and other wierd cases where the 
default configuration is missing). People who used the app before this 
change will not have a `default-config` entry in their indexedDB, 
courtesy of main-controller.js:224-233
Create toast with reload option instead of alert()
When no default configuration was found, create a new toast using the 
toastUI rather then using the standard `alert()`. The toast also allows 
for easy implementation of a reload option in the notification.
@jakearchibald

This comment has been minimized.

Copy link
Owner

jakearchibald commented Jul 24, 2018

Ohh thanks! Will take a look.

@jakearchibald

This comment has been minimized.

Copy link
Owner

jakearchibald commented Jul 24, 2018

There is currently no "Are you sure" popup when clicking the reset configuration button, might be an idea to add it

I think the best way to do this is via a toast (https://github.com/jakearchibald/svgomg/blob/master/src/js/page/main-controller.js#L158) which says "Options reset" but has "Undo" & "Dismiss" buttons. I'll take a look at that.

@ericcornelissen

This comment has been minimized.

Copy link
Author

ericcornelissen commented Aug 1, 2018

I think the best way to do this is via a toast (https://github.com/jakearchibald/svgomg/blob/master/src/js/page/main-controller.js#L158) which says "Options reset" but has "Undo" & "Dismiss" buttons. I'll take a look at that.

I've already used a toast, so I'm comfortable adding it for you 馃槃

ericcornelissen added some commits Aug 1, 2018

Add "Undo" toast so the reset can be reverted
Adds an "Undo" option to the existing toast if no default configuration 
was found, and a new toast with an "Undo" and "Dismiss" button if an 
existin default configuration *was* found.
Implement undo feature for reset configuration
Naive implementation to restore the config before resetting the 
configuration
Reduce code duplication
By reusing _saveSettings in the _resetConfig method, slightly altering 
it to work as expected, as _resetConfig sets/saves the configuration on 
three seperate occasions.

_saveSetting now also sets the config in the settings UI and compresses 
the SVG.
@jakearchibald

This comment has been minimized.

Copy link
Owner

jakearchibald commented Aug 7, 2018

I'm going to have a hack around with this. I think the setting is currently in the wrong place 鈥 it should sit with the settings it resets. There might be a better way to encapsulate the behaviour too. I'll have a play and get you to review if that's ok?

@ericcornelissen

This comment has been minimized.

Copy link
Author

ericcornelissen commented Aug 7, 2018

Of course it is, have a go at it 馃槃

@jakearchibald

This comment has been minimized.

Copy link
Owner

jakearchibald commented Aug 7, 2018

#142 - how's this?

@ericcornelissen

This comment has been minimized.

Copy link
Author

ericcornelissen commented Aug 7, 2018

#142 - how's this?

Looks good 馃憤

@jakearchibald

This comment has been minimized.

Copy link
Owner

jakearchibald commented Aug 7, 2018

Done in #142. Thanks @ericcornelissen for helping with the design!

@ericcornelissen

This comment has been minimized.

Copy link
Author

ericcornelissen commented Aug 7, 2018

Np, glad it is added now 馃帀

@ericcornelissen ericcornelissen deleted the ericcornelissen:reset-configuration branch Aug 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment