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.

Sign up for GitHub

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

Jump to bottom

Abstract Away Faux Modules from MapBlock Component and make renderable for NextJs Instance Prototype #1046

Merged
merged 9 commits into from
Jul 26, 2024
Merged

Abstract Away Faux Modules from MapBlock Component and make renderable for NextJs Instance Prototype #1046

merged 9 commits into from
Jul 26, 2024

Conversation

sandrahoang686
Copy link
Collaborator

@sandrahoang686 sandrahoang686 commented Jul 11, 2024 •
edited
Loading

Related Ticket: #1002

These changes are RELATED TO THIS PR: developmentseed/next-veda-ui#1

Description of Changes

This makes the MapBlock component renderable by the NextJs instance by abstracting away the veda faux modules dependencies. This also exports the themeProvider and reactQueryProvider to be used by the nextJs instance.

I've tried to keep these changes quite isolated to the problem we are trying to solve so its not too big of a PR.

Notes & Questions About Changes

  • Should NOT BE BREAKING CHANGES. Should be able to merge without care of order of NextJs PR
  • See my comments in the code

Validation / Testing

  • Regular build should work
  • Library build should work
  • The stories and mapblocks in the stories should render as usual in the deploy preview

@netlify Netlify
Copy link

netlify bot commented Jul 11, 2024 •
edited
Loading

✅ Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 72193b0
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/66a1085380858c0008c1c392
😎 Deploy Preview https://deploy-preview-1046--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

Thanks for putting various works on making the component work with the NEXT instance. I could see how it was unavoidable to make multiple changes when making changes for the component library. Once we are clear on what needs to be done, can we make the changes more gradually? I see there are several kinds of changes here.
Some changes I could spot are

  1. Make the types come from non-faux module
  2. Make the utility functions not to depend on faux-module (ex. additional parameters added for utility functions to accept the list of the datasets)
  3. Make the change/adjustments for the components needed to be used in the library context.

How does this sound?

sandrahoang686
sandrahoang686 commented Jul 16, 2024
View reviewed changes
app/scripts/components/common/map/utils.ts
import { Map as MapboxMap } from 'mapbox-gl';
import { MapRef } from 'react-map-gl';
import { endOfDay, startOfDay } from 'date-fns';
import startOfDay from 'date-fns/startOfDay';
import endOfDay from 'date-fns/endOfDay';
Copy link
Collaborator Author

@sandrahoang686 sandrahoang686 Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Parcel build for library was unable to find these date-fns if it wasn't imported like this... So had to switch over to doing this. Seems to be an open issue with Parcel
parcel-bundler/parcel#9676

sandrahoang686
sandrahoang686 commented Jul 16, 2024
View reviewed changes
package.json Outdated Show resolved Hide resolved
@sandrahoang686 sandrahoang686 requested a review from dzole0311 July 16, 2024 01:05
@sandrahoang686 sandrahoang686 marked this pull request as ready for review July 16, 2024 01:05
@sandrahoang686 sandrahoang686 changed the title Make MapBlock renderable for NextJs instance Abstract away Faux Modules from MapBlock Component and make renderable for NextJs Instance Prototype Jul 16, 2024
@sandrahoang686 sandrahoang686 changed the title Abstract away Faux Modules from MapBlock Component and make renderable for NextJs Instance Prototype Abstract Away Faux Modules from MapBlock Component and make renderable for NextJs Instance Prototype Jul 16, 2024
@hanbyul-here
Copy link
Collaborator

hanbyul-here commented Jul 18, 2024 •
edited
Loading

👋

I could narrow down what component leads to a build failure by commenting out the components we are exporting gradually. I could check that the build only fails when MapBlock component is used. (Parcel still has some complaints about ts but still builds the library.)

When we look at the error log with Mapblock, there are several kinds of errors. To point out the most suspicious one:

@parcel/transformer-typescript-types: Cannot find module 'veda' or its corresponding type declarations. - Is there any utility function that is still using 'veda' module? We unfortunately need to check the dependency of the dependency recursively. ex. BlockMap is using types from exploration/types.d.ts. : https://github.com/NASA-IMPACT/veda-ui/pull/1046/files#diff-b93e783f2167705031fb25f815ba97972896110f71e42d83e32fd6e557d75bccR34 and that files still has veda faux module as a dependency: https://github.com/NASA-IMPACT/veda-ui/blob/main/app/scripts/components/exploration/types.d.ts.ts#L1

I suggest reviewing BlockMap's dependencies to ensure it is completely independent from the faux module. Then, we can see if other errors still interrupt the library build.

Also, it might be worth merging the main branch since some major updates of Parcel went into the main recently!

@sandrahoang686 sandrahoang686 force-pushed the update/prop-driven-data-exp-#1002 branch from 29a313d to ed87f13 Compare July 22, 2024 13:18
dzole0311
dzole0311 approved these changes Jul 25, 2024
View reviewed changes
Copy link
Collaborator

@dzole0311 dzole0311 left a comment

Choose a reason for hiding this comment

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

The changes look good to me, and I also tested the Stories, Scrollytelling, Datasets and E&A and couldn't spot any issues.

hanbyul-here
hanbyul-here approved these changes Jul 26, 2024
View reviewed changes
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.

Yay, thanks for the work 🙇

I could also export CatalogView component without problem on this branch. I am excited for the next step!

@sandrahoang686 sandrahoang686 merged commit f58b9e1 into main Jul 26, 2024
8 checks passed
@sandrahoang686 sandrahoang686 deleted the update/prop-driven-data-exp-#1002 branch July 26, 2024 14:59
@sandrahoang686 sandrahoang686 mentioned this pull request Jul 30, 2024
Merged
sandrahoang686 added a commit that referenced this pull request Jul 31, 2024
@sandrahoang686
[RELEASE] Bump to v5.4.0 (#1074)
a59bd0a 
**HOLD until we merge #1073

## 🎉 Features
* Added Announcement component using USWDS Design System and adding
purge-css #1031

## 🚀 Improvements
* Add logic for tracking out-of-view Timeline playheads for E&A page
#1067
* Abstract Away Faux Modules from MapBlock Component and expose as
library component #1046

## 🐛 Fixes
- 🦗
@sandrahoang686 sandrahoang686 mentioned this pull request Oct 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzole0311 dzole0311 dzole0311 approved these changes

@hanbyul-here hanbyul-here hanbyul-here approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sandrahoang686 @hanbyul-here @dzole0311