-
Notifications
You must be signed in to change notification settings - Fork 959
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 dapp-draft icon in deployed contract #4608
Conversation
👷 Deploy request for remixproject pending review.Visit the deploys page to approve it
|
@@ -390,7 +390,7 @@ class PluginLoader { | |||
localStorage.setItem('workspace', JSON.stringify(saved)) | |||
}, | |||
get: () => { | |||
return JSON.parse(localStorage.getItem('workspace')) | |||
return JSON.parse(localStorage.getItem('workspace') || '[]').filter(item => item !== 'dapp-draft') |
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 explain thy this change?
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 think I made a mistake here. I was not sure whether or not the dapp-draft plugin should be loaded in main panel, when I reload remix.
I just checked other main panel plugins, they were loaded when I reload remix. So I guess it's fine for dapp-draft too.
I will recover this change soon. @yann300
@@ -103,12 +103,20 @@ export const setupEvents = (plugin: RunTab, dispatch: React.Dispatch<any>) => { | |||
if (plugin.name === 'remixd') { | |||
dispatch(setRemixDActivated(true)) | |||
} | |||
|
|||
if (plugin.name === 'dapp-draft') { |
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 m not sure why you need to do this.. when the user click on the icon to load dapp-draft, the plugin will be automatically activated.
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.
so basically you don't need to check whether the plugin is activated or not.
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'm not sure. I mean it's not a native plugin for now. Are you ok with the icon always show in deployed contract? I definitely don't need this if you're ok with that. @yann300
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.
If this requires the plugin to be activated I fear no one will see it. So, let's have this always displayed for now. As long as the feature is free to use, and doesn't imply any subscription, I think we'll be fine for that.
@LianaHus might want to check the UI here though.
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.
This feature will always be free to use and doesn't imply any subscription. I just removed isActived check. @yann300
dappdraftActivated={runTab.dappdraftActivated} | ||
exEnvironment={runTab.selectExEnv} | ||
editInstance={(instance) => { | ||
plugin.call('dapp-draft', 'edit', {address: instance.address, abi: instance.contractData.abi, name: instance.name, network: runTab.networkName}) |
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.
from the top of my head, I think just this will work.
<label> | ||
<b><FormattedMessage id="udapp.balance" />:</b> {instanceBalance} ETH | ||
</label> | ||
{props.exEnvironment === 'injected' && <i className="fas fa-edit btn btn-sm p-0" onClick={() => {props.editInstance(props.instance)}}></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.
I think this should also be displayed for all providers except VM providers. But what do you think?
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.
Yeah, I think so. But the dapp template only works for intected provider for now. More providers will be adapted in further iterations. @yann300
Can I put dapp-draft in featured plugins? @yann300 |
add an edit icon in deployed contract to automatically input dapp parameters.
@ryestew @yann300