Feature multiple sources #256
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chunfantasy
reviewed
Oct 3, 2018
| @@ -46,7 +46,9 @@ const AvailableAppItem = ({ | |||
| isDisabled, | |||
| }) => ( | |||
| <div className="core-app-management-item list-group-item"> | |||
| <h4 className="list-group-item-heading">{app.displayName || app.name}</h4> | |||
| <h4 className="list-group-item-heading">{app.displayName || app.name} | |||
| <span className="list-group-item-heading-sourcetag">{app.source}</span> | |||
There was a problem hiding this comment.
Maybe s/sourcetag/source-tag ?
| <table className="core-settings-sources"><tbody> | ||
| { | ||
| Object.keys(sourcesJS) | ||
| .filter(name => name !== 'official') |
There was a problem hiding this comment.
This indentation is automatic, can't force it other way.
| import * as SettingsActions from '../actions/settingsActions'; | ||
|
|
||
| const InitialState = Record({ | ||
| shouldCheckForUpdatesAtStartup: true, | ||
| isUpdateCheckCompleteDialogVisible: false, | ||
| isLoading: false, | ||
| sources: Map({}), |
There was a problem hiding this comment.
Question, why use Map for sources?
There was a problem hiding this comment.
Because a simple key, value container is needed there and this one works.
main/apps.js
Outdated
| .catch(error => { | ||
| throw new Error(`Unable to download apps list: ${error.message}. If you ` + | ||
| 'are using a proxy server, you may need to configure it as described on ' + | ||
| 'https://github.com/NordicSemiconductor/pc-nrfconnect-core'); | ||
| }) | ||
| .then(appsJson => { | ||
| const source = appsJson._source; // eslint-disable-line |
There was a problem hiding this comment.
Is it possible to remove '_'?
There was a problem hiding this comment.
The underscore comes from Json which is intentional as a meta tag, I'll comment about it.
main/settings.js
Outdated
| if (sourcesData !== null) { | ||
| return; | ||
| } | ||
| let sources = parseJSONFile(config.getSourcesJsonPath()); |
There was a problem hiding this comment.
How about s/parseJSONFile/parseJsonFile since all the other places are written as 'Json'
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a new section to the the SettingsView where users can add multiple apps.json URLs for external app sources. nRFConnect creates a new directory
.nrfconnect-apps/externalwhich will be the root of storing these apps.There's an extra requirement for any additional apps.json, to specify
_sourcestring member as the source name which will create a new directory underexternal.New command line argument
sourcehas been introduced to be able to specify the source which works together withopen-official-app. This change is also added to desktop shortcuts.Jest tests and snapshots are updated.