-
Notifications
You must be signed in to change notification settings - Fork 5
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
IPFS hash to link and creator address #49
Conversation
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.
Looks good!
Left a couple of suggestions and we should also keep the creator
between delay script updates.
const { timestamp } = await getBlock(blockNumber) // TODO: Move to getDelayedScript (keep in mind that updateScript also calls this function) | ||
const delayedScript = { | ||
...(await getDelayedScript(scriptId, blockNumber)), | ||
timeSubmitted: marshallDate(timestamp), | ||
totalTimePaused: 0, | ||
creator, |
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.
We should also save this prop when the script is updated (e.g when the script is paused or resumed) otherwise we would lose this information.
This can be achieved at
Line 186 in b50f493
function mergeScripts(oldScript, newScript) { |
creator
from oldScript
app/src/components/ScriptText.js
Outdated
@@ -34,7 +36,14 @@ const ScriptText = React.memo( | |||
<LocalIdentityBadge badgeOnly={disabled} compact entity={part} /> | |||
</span> | |||
) : ( | |||
<span key={index}>{part}</span> | |||
transformIPFSHash(part, (word, isIpfs, i) => { |
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.
Should we reword isIpfs
to isIpfsHash
for consistency? Also i think we could wrap all this into a <React.Fragment />
and pass as key the index from transformAddress
And then use this i
index to the components we render below <Link />
or <span/>
app/src/components/ScriptText.js
Outdated
const ipfsUrl = generateURI(word) | ||
return isIpfs ? ( | ||
<Link href={ipfsUrl}>{ipfsUrl} </Link> | ||
) : ( | ||
<span key={index}>{word}</span> | ||
) | ||
}) |
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.
What do you think of reafctoring this to something like:
const ipfsUrl = generateURI(word) | |
return isIpfs ? ( | |
<Link href={ipfsUrl}>{ipfsUrl} </Link> | |
) : ( | |
<span key={index}>{word}</span> | |
) | |
}) | |
if (isIpfs) { | |
const ipfsUrl = generateURI(word) | |
return <Link href={ipfsUrl}>{ipfsUrl} </Link> | |
} | |
return <span key={index}>{word}</span> | |
}) |
So we don't generate the URI in case it's not an ipfsHash
this closes #47 #48