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

Add suffix props for RasterTimeseries component #543

Merged
merged 4 commits into from Jun 2, 2023
Merged

Conversation

hanbyul-here
Copy link
Collaborator

@hanbyul-here hanbyul-here commented May 25, 2023

Closes #542

This PR is a quick fix to add suffixes to rastetimeseris component for Scrollymap component. We would need a way of generating unique IDs if we are moving forward in the direction of displaying multiple layers simultaneously in the future. I feel like that is an overengineering at this moment so here is a quick fix.

@netlify
Copy link

netlify bot commented May 25, 2023

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 8169d2e
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/6478f33bf3474e0008085c98
😎 Deploy Preview https://deploy-preview-543--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 settings.

@danielfdsilva
Copy link
Collaborator

@hanbyul-here Can you share an example of where the problem was happening?

@hanbyul-here
Copy link
Collaborator Author

The bug can be observed through any scrollymap component. Ex. Scrolly map component on this discovery failing to load the first layer: https://www.earthdata.nasa.gov/dashboard/discoveries/fire-life-cycle

Copy link
Collaborator

@danielfdsilva danielfdsilva left a comment

Choose a reason for hiding this comment

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

I think this works as a quick fix to get the stories working again. 👍
The suffix also needs to be include in the unmount useEffect.

However this makes the layer component and the generatorId prop a bit confusing. Could you open an issue where you detail the problem, explain what the fix does and what could be next steps? I think we should clean this tech debt as soon as possible.

Copy link
Collaborator

@danielfdsilva danielfdsilva left a comment

Choose a reason for hiding this comment

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

Missed the dependency in the unmount useEffect

@hanbyul-here
Copy link
Collaborator Author

hanbyul-here commented May 31, 2023

@danielfdsilva Yeah I can make a follow-up ticket but can you elaborate on how this makes generatorId confusing?
Is this about having something more scalable? ex. object hash-based dynamic id generator? or more like other layers not having the same ID structure (ex. vector series doesn't accept suffix). Or is it something else?

@danielfdsilva
Copy link
Collaborator

@hanbyul-here hmm, I think the vector should also have the suffix. For now we don't have them in the scrolly, but they could be there.

The generatorId was referring to what "created" that layer i.e. the type of component. Perhaps renaming it to something else would suffice? Or maybe I'm just over complicating.
@nerik what was your reasoning when you created this?

@hanbyul-here
Copy link
Collaborator Author

This PR is ready @danielfdsilva

@hanbyul-here hanbyul-here merged commit 34278af into main Jun 2, 2023
9 checks passed
@hanbyul-here hanbyul-here deleted the fix/scrolly branch June 2, 2023 16:51
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.

Scrollytelling + raster-timeseries problems
2 participants