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
GH-1691: Add import/export menu in client app #1728
GH-1691: Add import/export menu in client app #1728
Conversation
@@ -122,7 +122,7 @@ class MenuManager { | |||
click: () => this.actionManager.exportAppData(), | |||
}, | |||
{ | |||
label: 'Restore data...', | |||
label: 'Import data...', |
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.
For consistency
@@ -6,9 +6,6 @@ import { Injectable } from '@angular/core'; | |||
import { Store, Action } from '@ngrx/store'; | |||
import { createEffect, Actions, ofType } from '@ngrx/effects'; | |||
|
|||
import * as fromRoot from '../store'; | |||
import * as fromWindows from '../store/windows/windows.reducer'; |
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.
Not used
@@ -1,7 +1,5 @@ | |||
import { Action as NGRXAction } from '@ngrx/store'; | |||
|
|||
import * as fromWindows from './windows.reducer'; |
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.
Not used
[nzDropdownMenu]="dataMenu" | ||
class="app-header__menu-item app-header__menu-item--icon" | ||
> | ||
<app-icon name="hard-drive"></app-icon> |
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.
I don't particularly like this icon but the offering isn't that great. It was either this or the database one.
I wonder if these menu items should be kept somewhere else as opposed to being visible directly in the header? |
Not sure to be honest. I was thinking of just putting them in the already-existing settings menu as that has some import options. They are not really settings so in the end I went for a different menu. How do you feel about moving the two import options from the settings menu to this new one? This way, all the import/export get their own menu and the settings one stays for other stuff. |
@@ -347,6 +348,16 @@ export class AppComponent { | |||
this.store.dispatch(new windowActions.ReopenClosedWindowAction()); | |||
} | |||
|
|||
exportData() { | |||
this.electronApp.exportBackupData(); |
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.
Could you create new actions for these and add this logic to the effect?
How about we still add them under the settings menu but being specific about the "backup" part? We could also keep it at the bottom of the menu, so it's separate from the other import settings. I'm still trying to figure out the best way to minimize the number of buttons in the UI (trying to maintain a simple-r UI), so not also sure what the best solution would be for this |
I've moved the two functions to the settings menu. Let me know if you disagree with the use of 'backup' anywhere. |
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.
Amazing stuff! It all looks perfect. Just have comments about the use of console.log
.pipe( | ||
ofType(windowsMetaActions.EXPORT_BACKUP_DATA), | ||
switchMap(() => { | ||
console.log('exportBackupData'); |
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.
Let's use the debug.log
instead. That makes sure there are no logs in production.
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.
Those were not meant to be there. I keep forgetting to remove the debug stuff recently. I'll take them out.
.pipe( | ||
ofType(windowsMetaActions.IMPORT_BACKUP_DATA), | ||
switchMap(() => { | ||
console.log('importBackupData'); |
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.
Let's use the debug.log
instead. That makes sure there are no logs in production.
Fixes
#1691
Checks
yarn test-build
Changes proposed in this pull request:
Adds a small menu before the settings one that lets you export and import your data similar to the one offered by the browser extensions
screencast-2021-10-23-004217_4uvFqyVP.mp4