-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add support for settings (picking root folders) #74
Conversation
This pull request introduces 4 alerts when merging a8535d5 into 5c6391a - view on LGTM.com new alerts:
|
a8535d5
to
cbd0426
Compare
This pull request introduces 4 alerts when merging cbd0426 into 5c6391a - view on LGTM.com new alerts:
|
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.
Nice refactor; looks good. A couple of wider issues:
- we should consider migrating AppState data from the old store if it exists to have a consistent UX Migrate AppState data from the old store #76
- would be good to have a reset-to-default for directories settings Add reset to default directory in the Settings drawer #75
cbd0426
to
29ce8e6
Compare
This pull request introduces 4 alerts when merging 29ce8e6 into bca7d15 - view on LGTM.com new alerts:
|
29ce8e6
to
c6a4097
Compare
This pull request introduces 4 alerts when merging c6a4097 into 845898e - view on LGTM.com new alerts:
|
This is cleaner than manually persisting to local storage.
Commit body Change-type: patch|minor|major Signed-off-by: Graham McCulloch <graham@balena.io>
Commit body Change-type: patch|minor|major Signed-off-by: Graham McCulloch <graham@balena.io>
Note: The implementation in sources/scripture-app-builder.js is still required before any Scripture App Builder projects will be returned. At the moment we just return an empty array!
Refactor mobx store to contain two store classes, appState and settings.
* Updating settings will trigger a projects refresh. * Settings button is disabled while processing.
c6a4097
to
c3ad73a
Compare
This pull request introduces 4 alerts when merging c3ad73a into 845898e - view on LGTM.com new alerts:
|
This PR adds basic support for Settings - current just allows the user to specify the HearThis and Scripture App Builder root directories.
(This is branched off the
scripture-app-builder-ui
branch so that should be completed and merged before we merge this one).