-
Notifications
You must be signed in to change notification settings - Fork 34
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
[WIP] CRA to Next (Q1-Q2 roadmap) #35
base: main
Are you sure you want to change the base?
Conversation
Deploy request for orcamap accepted. Accepted with commit e0bb347 https://app.netlify.com/sites/orcamap/deploys/607151ccb8dd9400072ff5ff |
@ivanoats a large number of deletions are showing because of package-lock addition of next.js, is there a fix to that ? |
1).I think that OL is better than react-gl. Its vector layers are better and a plus point. 2)As We proceed Forward to use APIs or googlesheets and also data handling patterns like context.. Your folder structure complies so i think its great.. |
… plugin to transpile modules Removed react-scripts and added babel plugins to transpile modules, preset-env
…nore, eslintignore for config Addition of nextconfig file, removing it from eslist run and using it to transpile modules and plugins BREAKING CHANGE: Addition of next config to build and run the environment
…r next requirements Folder refactor, global css files are placed in public directory, __app is created to import global styles places in styles directory, removing old react config, changing eslint to support next directory requirements
…nents for nextjs serving page Addition of functional headers and layours to render individual components
…imports Changes mapcontainer to support nextjs, next-images to support image import inside the container, addition of olmin.css as 'map.module.css' fixed image imports in vector layer and google sheets layer, added public assets, fixed eslint config to remove next.config.js BREAKING CHANGE: Tilelayer after update needs to be triggered manually to be re-rendered for the first time, upon addition of custom width using inspect the code works as expected. Need to add serviceworker, jest config for testing and ensure tilelayer re-render after mapupdate in useeffect
0f130d5
to
e0bb347
Compare
@ivanoats update of current active branch,
Current implementation
Bugs introduced
|
Following #27 .
Issue
Next-js proposal for addition for react-map following suggestion of @ivanoats
Benefits
Suggestions of sahil-shubham
Image Optimization - Nextjs offers a component named "Image" which brings features like lazy loading, quality, priority for loading of specific images etc. but at the moment it also breaks any build which uses the command next export, which is also necessary (we can remove it's need by making a node.js server) for deploying the site (currently orcamap-react is hosted on netlify). It happens because next on itself cannot optimize images on build (hence the need of node.js server)
Issue mentioning the same - _next/image 404 on next build && next export vercel/next.js#18356
Conversation requesting the feature request - Optimize images during `next build` vercel/next.js#19065
Work done
/public
directory tosrc
using https://nextjs.org/docs/advanced-features/src-directory( need recommendation for this change moved it considering it as static data, inside public folder)
next
to support nextjsnext-images
to import static images without usingnext/images
see issue https://stackoverflow.com/questions/59546370/i-cant-reference-an-image-in-next-js.react-script
eject
script (not supported in next yet) see Next.js eject vercel/next.js#1691. Accordingly Readme.md can also be corrected to remove reference for eject scriptCurrent Status
After porting complete application, reached a roadblock since
openlayers
essentially used to plot points in orcamap, doesn't support SSR ( used in next.js), tried few fixes of webpack configuration and support from openlayers devs provided a fix it correctly. Currently testingtranspile-module
to ensure desired results.WIP
Currently project would not compile due to openlayers/openlayers#10470 (comment), but will be corrected in few days.
Mergability
Branch was checkout from
main
branch and might lead to merge conflicts.nextjs
standards. Ideallly creating amodule.css
extension.Observations
@ivanoats @scottveirs openlayers/openlayers#10470 (comment) shows working POC of SSR openlayers but isn't stable.
current
transpile-module
would fix the issue.Further we should also try
react-map-gl
mapbox library.See https://www.geoapify.com/map-libraries-comparison-leaflet-vs-mapbox-gl-vs-openlayers-trends-and-statistics for comparision and choice decision as well.
@ivanoats do let me know about your response about.