-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Template Explorer Modal #6378
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
base: master
Are you sure you want to change the base?
Template Explorer Modal #6378
Conversation
✅ Deploy Preview for reliable-cocada-166884 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
0d1cdf1 to
a393474
Compare
dd34dec to
d4386ee
Compare
| version: packageJson.version, | ||
| icon: '', | ||
| location: 'none', | ||
| methods: ['getWorkspaces', 'createWorkspace', 'renameWorkspace', 'deleteWorkspace', 'getCurrentWorkspace', 'setWorkspace'], |
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.
can you explain 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.
the plugin doesn't fit in either mainpanel, sidePanel so I used none like remixAIPlugin for example
| > | ||
| { section.cells.map((cell, index) => { | ||
| return <RemixUIGridCell | ||
| plugin={this} |
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.
is that change related?
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.
yes see comment below. tldr is plugin is not used in the component
| } | ||
|
|
||
| console.log('templates', templates(window._intl, this)) | ||
| return ( |
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.
remove log
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.
removed
| <RemixUIGridCell | ||
| plugin={this} | ||
| // plugin={this} | ||
| title={item.displayName} |
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.
same
| <platformContext.Provider value={props.app.platform}> | ||
| <onLineContext.Provider value={online}> | ||
| <AppProvider value={value}> | ||
| {isAiWorkspaceBeingGenerated && <AiWorkspaceGeneration></AiWorkspaceGeneration>} |
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.
are you still using this? AiWorkspaceGeneration
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 is now removed
| {section.providers.map(provider => ( | ||
| <RemixUIGridCell | ||
| plugin={this} | ||
| // plugin={this} |
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.
why 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.
plugin is not used in the GridCell component. I found it confusing when I was reading the code and commented it out so I would understand that it isn't being used. see
| props.plugin.on(props.plugin.name, 'expandGridCell', listenOnExpand) |
apps/remix-ide/src/app/plugins/remix-template-explorer-modal.tsx
Outdated
Show resolved
Hide resolved
| }, []) | ||
|
|
||
| useEffect(() => { | ||
| console.log('state', state) |
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.
remove log
| const facade = new TemplateExplorerModalFacade(plugin, appContext, dispatch, state) | ||
| const templateCategoryStrategy = new TemplateCategoryStrategy() | ||
|
|
||
| useEffect(() => { |
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.
can we merge code in some UseEffects here?
| }, [strategy.contractType, strategy.contractOptions, strategy.contractAccessControl, strategy.contractUpgradability, strategy.contractName, strategy.contractTag]) | ||
|
|
||
| useEffect(() => { | ||
| console.log('strategy', strategy) |
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.
remove log
| }, [strategy]) | ||
|
|
||
| const switching = (value: 'erc20' | 'erc721' | 'erc1155') => { | ||
| console.log('switching', value) |
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.
remove log
| useEffect(() => { | ||
| const run = async () => { | ||
| const readMe = await facade.getTemplateReadMeFile(state.workspaceTemplateChosen.value) | ||
| console.log('readMe', readMe) |
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.
remove log
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.
done
libs/remix-ui/template-explorer-modal/src/components/genericWorkspaceTemplate.tsx
Show resolved
Hide resolved
libs/remix-ui/template-explorer-modal/src/components/genericWorkspaceTemplate.tsx
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,162 @@ | |||
| import React, { useState } from 'react' | |||
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.
name of this file is not very well signifying the usage of it. Either update the name or add comment on the top of file to explain its usage
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.
added comments to the component
| {dedupedTemplates?.map((template: TemplateCategory, templateIndex) => ( | ||
| <div key={template.name} className="template-category mb-4" data-id={`template-category-${template.name}`}> | ||
| <h4 className="category-title mb-3 text-dark" style={{ | ||
| color: '#667', |
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 possible, avoid this much inline styling. Use built-in classed when possible
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.
some most of these there are no bootstrap classes for them, hover states and the like. I have cleaned this up and added them to the css file for the react library.
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.
done
| data-id={`template-card-${item.value}-${itemIndex}`} | ||
| key={`${templateIndex}-${itemIndex}`} | ||
| className="template-card bg-light border-0" | ||
| style={{ |
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.
same here & below at multiple places in this file
| if (!remixconfigExists) { | ||
| await plugin.call('fileManager', 'writeFile', '/remix.config.json', JSON.stringify({ project: targetFolder, version: '1.0.0', IDE: window.location.hostname }, null, 2)) | ||
| } | ||
| if (!remixconfigExists1) { |
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.
you check if file doesn't exist then remove it. This logic is weird. This implementation needs improvement.
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 has since been fixed.
| dispatch({ type: ContractWizardAction.CONTRACT_TYPE_UPDATED, payload: value }) | ||
| dispatch({ type: ContractWizardAction.CONTRACT_TAG_UPDATE, payload: value.toUpperCase() }) | ||
| dispatch({ type: TemplateExplorerWizardAction.SET_WORKSPACE_NAME, payload: value === 'erc20' ? 'ERC20' : value === 'erc721' ? 'ERC721' : 'ERC1155' }) | ||
| dispatch({ type: TemplateExplorerWizardAction.SET_WORKSPACE_TEMPLATE, payload: value === 'erc20' ? { value: 'ozerc20', displayName: 'ERC20', tagList: ["ERC20", "Solidity"], description: 'A customizable fungible token contract' } : value === 'erc721' ? { value: 'ozerc721', displayName: 'ERC721', tagList: ["ERC721", "Solidity"], description: 'A customizable non-fungible token (NFT) contract' } : { value: 'ozerc1155', displayName: 'ERC1155', tagList: ["ERC1155", "Solidity"], description: 'A customizable multi token contract' } }) |
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 much code in one line using tertiary operator should be avoided
| } | ||
|
|
||
| if (rule.type === 'exactMatch') { | ||
| if (rule.field === 'displayName' && displayName === rule.value) { |
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.
these two if statements can be merged in one
Aniket-Engg
left a comment
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.
Creating a workspacce more than once doesn't show the right name on the modal but only after modal closing
can you explain a bit more. Not sure what you mean |
No description provided.