Upgrade discussions, improve documentation #1764
Conversation
javieralaves
changed the title
|
Hey @Schwartz10 could you please clarify if this one supersedes #1218 so we can close it? |
|
Yeah @ottodevs this one is good to pull in based on experimental-oe work |
|
|
||
| Thanks to [Chad Ostrowski](https://github.com/chadoh/) for this idea - instead of creating new discussions as a step in a forwarder chain, developers should be able to "create a dicsussion thread" on their own, and completely manage their ID schema in whatever way they choose. | ||
|
|
||
| This would require some under the hood shifts, but should be possible. To handle this feature change, the discussions app should prepend the app identifier to the ID before creating a discussion thread to avoid namespace conflicts: `[appIdentifier][discussionId]`. |
There was a problem hiding this comment.
That could potentially result in multiple discussionIds assigned to the same appIdentifier, that could end up being a bit chaotic, how would we handle this potential issue?
| @@ -133,13 +152,13 @@ const Comment = ({ | |||
| onSave={update} | |||
| /> | |||
| ) : ( | |||
| <React.Fragment> | |||
| <Top author={author} createdAt={createdAt} /> | |||
| <Fragment> | |||
There was a problem hiding this comment.
Not a big deal, but just note that recent react versions allow to omit the Fragment part so you can just type it as:
| <Fragment> | |
| <> |
| setHandshakeOccured(true) | ||
| } | ||
| } | ||
| const handleWrapperMessage = ({ data }) => setHandshakeOccured(true) |
There was a problem hiding this comment.
Minor misspelling
| const handleWrapperMessage = ({ data }) => setHandshakeOccured(true) | |
| const handleWrapperMessage = ({ data }) => setHandshakeOccurred(true) |
| }, | ||
| "peerDependencies": { | ||
| "@aragon/api": "2.0.0-beta.8", | ||
| "@aragon/api-react": "2.0.0-beta.6", |
There was a problem hiding this comment.
api-react must be updated to at least beta.9 as well
| @@ -50,10 +52,10 @@ | |||
| "start:app": "cd app && npm start && cd ..", | |||
| "test": "cross-env TRUFFLE_TEST=true npm run ganache-cli:test", | |||
| "compile": "aragon contracts compile", | |||
| "build": "npm run sync-assets && npm run build:app && npm run build:script", | |||
| "build": "npm run compile && npm run sync-assets && npm run build:app && npm run build:script", | |||
There was a problem hiding this comment.
Compile step is probably not needed here, as it is part of the regular build during aragon apm publish so it would eventually run twice
| @@ -62,7 +64,8 @@ | |||
| "lint": "solium --dir ./contracts", | |||
| "lint:fix": "eslint . --fix && solium --dir ./contracts --fix", | |||
| "coverage": "cross-env SOLIDITY_COVERAGE=true npm run ganache-cli:test", | |||
| "ganache-cli:test": "sh ./node_modules/@aragon/test-helpers/ganache-cli.sh" | |||
| "ganache-cli:test": "sh ./node_modules/@aragon/test-helpers/ganache-cli.sh", | |||
There was a problem hiding this comment.
This script will probably be located at the root folder as long as we use the hoisted version for test-helpers
| "ganache-cli:test": "sh ./node_modules/@aragon/test-helpers/ganache-cli.sh", | |
| "ganache-cli:test": "sh ../node_modules/@aragon/test-helpers/ganache-cli.sh", |
No description provided.