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

Fix critical error on /exploration #709

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

danielfdsilva
Copy link
Collaborator

The error identified in #708 was introduced by the commit 6721546.

const componentName = (child.type as JSXElementConstructor<any>).name;
if (componentName === 'Compare') {
acc.compareGenerators = Children.toArray(
child.props.children
) as ReactElement[];
} else if (componentName.endsWith('Control')) {
acc.controls = [...acc.controls, child];
} else {
acc.generators = [...acc.generators, child];

The code here uses the name property (which is derived from the function name) to understand what to do with the component being rendered. The problem is that in a production environment, the code is minified and function names change making this property unreliable and unpredictable.

The solution I found was to use the displayName property that react allows us to attach to components to give them some names. This is a paradigm we already use in the app with the blocks for the scrollytelling (eg1, eg2)

The only thing to keep in mind is that if we create a new map control, we have to give it a displayName.

@netlify
Copy link

netlify bot commented Oct 24, 2023

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 9738d47
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/65394114a697f80007b27a59
😎 Deploy Preview https://deploy-preview-709--veda-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hanbyul-here
Copy link
Collaborator

I feel like I am missing the bigger context. If we need to sort out the components based on their display names in the background, why are we just passing them as explicit props?

@danielfdsilva
Copy link
Collaborator Author

@hanbyul-here all the components are added as children of Map, but then depending on what they do (Layer, Control, Compare config) they need to be processed differently, hence this code. This was Erik's implementation of how to abstract away the complexity of the different layer types we have.

@hanbyul-here
Copy link
Collaborator

It will be ideal if we don't have to name controls manually + have a coherent way of handling the children of Map. This : https://github.com/NASA-IMPACT/veda-ui/pull/711/files is what I thought out of it. (Having one container element that holds all the controls) how do you think?

@danielfdsilva
Copy link
Collaborator Author

@hanbyul-here I kind of like the current approach since it reduces wrappers and keeps a lean list of children. However the approach that you propose is exactly the same that is in place for the Compare so I think it makes more sense.
Will update the PR 👍

danielfdsilva added a commit that referenced this pull request Oct 25, 2023
Creating this to show the diff, will continue the convo in the originali
pr (#709)
@danielfdsilva
Copy link
Collaborator Author

@hanbyul-here Ready to review. Fixed the conflicts with the base branch.

Copy link
Collaborator

@hanbyul-here hanbyul-here left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix.

Not related to this PR, but do you see any problem making this map component (used by mdx map) : https://github.com/NASA-IMPACT/veda-ui/blob/main/app/scripts/components/common/mapbox/map.tsx to follow the same pattern?

@danielfdsilva
Copy link
Collaborator Author

@hanbyul-here The idea was to rebuild the map component with the learnings from that one and then apply it to the blocks.

So now, we should start refactoring the remaining maps in the app to use this new map component and then remove that old map altogether.

@danielfdsilva
Copy link
Collaborator Author

I'll open an issue for this

@danielfdsilva danielfdsilva merged commit 1c5bf02 into feature/exploration Oct 25, 2023
7 checks passed
@danielfdsilva danielfdsilva deleted the fix/exploration-error branch October 25, 2023 17:08
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

Successfully merging this pull request may close these issues.

2 participants