Skip to content
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

Refactor MessageTranslator component #105

Open
alexkeating opened this issue Mar 16, 2023 · 3 comments
Open

Refactor MessageTranslator component #105

alexkeating opened this issue Mar 16, 2023 · 3 comments
Assignees

Comments

@alexkeating
Copy link
Contributor

The switch statement within this component is getting a little crazy and tough to read. A more readable way to organize this component may be to accept a list of widget configurations and conditionally render a configuration based on the passed in widget function name. An example of what I am thinking of is below.

Note this is for a table component https://react.dev/reference/react/Children#accepting-an-array-of-objects-as-a-prop.

@kahkeng
Copy link
Collaborator

kahkeng commented Mar 17, 2023

Could you give an example of what you mean when you say widget configuration and the conditional rendering?

Right now, one consideration is that the *-container widgets that are being sent to the backend are potentially nested, e.g.:

The nft-asset-traits-container contains:
-asset: which is a nft-asset-container
-values: which is a list of nft-asset-trait-value-containers

So we render a full component composed of smaller components, but the composition is being configured at run-time by the backend.

@marcomariscal marcomariscal self-assigned this Mar 20, 2023
@alexkeating
Copy link
Contributor Author

Could you give an example of what you mean when you say widget configuration and the conditional rendering?

Right now, one consideration is that the *-container widgets that are being sent to the backend are potentially nested, e.g.:

The nft-asset-traits-container contains: -asset: which is a nft-asset-container -values: which is a list of nft-asset-trait-value-containers

So we render a full component composed of smaller components, but the composition is being configured at run-time by the backend.

Yes, this a purely code architecture thing and shouldn't affect any existing functionality. Instead of handling rendering of the components with a switch statement we will pass in an array of configs that have a function name and container/widget logic. And the component will search that array for the correct component to render. It should act the same way as the switch.

Below is an example of what I was thinking. For nested components we would need to make sure the configuration is passed into the lower level widgetize function. Definitely open to suggestions on how to improve this, but mainly wanted to highlight the need to refactor the switch logic

const widgetConfigurations = [{fnName: 'price', component: <PriceWidgetShell />}, {fnName: 'nft-asset-traits-container', component: <NftAssetTraitsContainerShell />}, ... ]

Widgetize(item, chain, widgetConfigurations) 

@kahkeng
Copy link
Collaborator

kahkeng commented Mar 23, 2023

Ok thanks for clarifying. I am not sure if it is going to be feasible without trying it out, but I think given Widgetize is currently called recursively, it is going to be tricky.

      case 'nft-collection-assets-container': {
        const { collection, assets } = JSON.parse(args);
        return (
          <NftCollectionAssetsContainer
            collection={Widgetize(
              { fnName: collection.name, args: JSON.stringify(collection.params) },
              chain
            )}
          >
            <div className="columns-1 text-black sm:columns-2">
              <ul role="list" className="divide-y divide-gray-200">
                {assets?.map(({ name, params }, i) => (
                  <Fragment key={`i${i}`}>
                    {Widgetize({ fnName: name, args: JSON.stringify(params) }, chain)}
                  </Fragment>
                )) || ''}
              </ul>
            </div>
          </NftCollectionAssetsContainer>
        );
      }

Consider the code above, given we know we're rendering a collection asset container, we know the structure of the args, that it contains a collection and a list of assets. Therefore, we can call Widgetize recursively on the collection and the assets, and pass them into the constructor and children prop of the NftCollectionAssetsContainer.

Thinking about it more, we probably could extract out the switch logic into a function that takes args and put that into the widgetConfigurations lis, instead of the component. The function will return the component but it might also call Widgetize again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants