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

feat: add extra maps #304

Closed
wants to merge 4 commits into from
Closed

feat: add extra maps #304

wants to merge 4 commits into from

Conversation

unclFedor
Copy link
Collaborator

No description provided.

@unclFedor unclFedor requested a review from belom88 October 23, 2023 11:21
@unclFedor unclFedor changed the title added new map components, fixed compilation errors, added vite feat: add arcgis maplibre googlemap and mapbox maps Oct 23, 2023
@unclFedor unclFedor changed the title feat: add arcgis maplibre googlemap and mapbox maps feat: add extra maps Oct 23, 2023
belom88
belom88 requested changes Oct 23, 2023
View reviewed changes
Copy link
Collaborator

@belom88 belom88 left a comment

Choose a reason for hiding this comment

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

A lot of excess code we should get rid of in this PR.

.env Outdated
Comment on lines 1 to 4
VITE_REACT_APP_MAPBOX_TOKEN=
VITE_REACT_APP_REST_API_BASE_URL=https://developer.trimet.org/ws/v2
VITE_REACT_APP_REST_API_APP_ID=
VITE_SOME_KEY=hello
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't use vite in this project. For env variables we have do define .env in the webpack config https://stackoverflow.com/questions/46224986/how-to-pass-env-file-variables-to-webpack-config

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. removed VITE_ prefix in the .env content
  2. uninstalled "vite"
  3. installed "dotenv". It seems "@types/dotenv" is deprecated
  4. updated webpack.dev.config.js

.env.local Outdated
Comment on lines 1 to 3
VITE_REACT_APP_MAPBOX_TOKEN=pk.eyJ1IjoiZHNhdmlub3YiLCJhIjoiY2xsd2VocjRsMjVpbzNwcDhsc3E1ZzAzNiJ9.3xSNq_gwuOMHPUTDkO4OzQ
VITE_REACT_APP_REST_API_BASE_URL=https://developer.trimet.org/ws/v2
VITE_REACT_APP_REST_API_APP_ID=97B28D2ED40DA08AB53687356
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't push API keys to the codebase. We have to add *.local to the .gitignore

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed .env.local

package.json Outdated
@@ -31,24 +38,29 @@
"@loaders.gl/tiles": "^4.0.0-beta.2",
"@luma.gl/core": "^8.5.14",
"@math.gl/proj4": "^3.6.3",
"@mui/material": "^5.14.13",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't use MUI in this project. Don't port UI components to this project

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. removed @mui/material from package.json
  2. removed loading.tsx, unsupported.tsx
  3. updated map-wrapper.tsx

package.json Outdated
"@probe.gl/stats": "^4.0.4",
"@reduxjs/toolkit": "^1.9.5",
"@types/google.maps": "^3.54.4",
"deck.gl": "^8.8.3",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like by-module import:

    "@deck.gl/arcgis": "^8.9.31",
    "@deck.gl/core": "^8.9.31",
    "@deck.gl/google-maps": "^8.9.31",

. Remove "deck.gl": "^8.8.3", then and add all required modules from @deck.gl/...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. removed "deck.gl": "^8.8.3"
  2. added
    "@deck.gl/geo-layers": "^8.9.31",
    "@deck.gl/layers": "^8.9.31",
    "@deck.gl/mesh-layers": "^8.9.31",
    "@deck.gl/react": "^8.9.31",
    "@loaders.gl/images": "^4.0.0-beta.2",
    "@loaders.gl/schema": "^4.0.0-beta.2",
    "apache-arrow": "^13.0.0",

src/components/deck-gl-wrapper/deck-gl-wrapper.tsx Outdated
Comment on lines 238 to 241
const baseMapProviderId =
baseMapProvider.id === BaseMapProviderId.maplibre
? BaseMapProviderId.maplibre
: BaseMapProviderId.mapbox2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Weird expression. If baseMapProvider.id is not maplibre , it can be google, arcgis or mapbox2. But it set mapbox2 for all those cases

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed expression. Changed type from String to BaseMapProviderId

src/components/interleaved-map/interleaved-map.tsx Outdated
@@ -0,0 +1,149 @@
import { Map as MapboxMap } from "mapbox-gl";
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need only overlaid mode. Don't port logic of interleaved mode

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed interleaved-map.tsx

src/components/loading/loading.tsx Outdated
@@ -0,0 +1,18 @@
import CircularProgress from "@mui/material/CircularProgress";
Copy link
Collaborator

Choose a reason for hiding this comment

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

MUI-related component. We don't need it here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Removed "@mui/material
  2. Removed loading .tsx

src/components/unsupported/unsupported.tsx Outdated
@@ -0,0 +1,28 @@
import Paper from "@mui/material/Paper";
Copy link
Collaborator

Choose a reason for hiding this comment

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

MUI-related component. We don't need it here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed unsupported.tsx

src/constants/notification-messages.ts Outdated
@@ -0,0 +1,9 @@
export const TERRAIN_WITH_SELECTED_BASE_MAP_PROVIDER_IS_NOT_SUPPORTED = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need notification messages in I3S Explorer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed notification-messages.ts

src/constants/view-states.ts Outdated
@@ -0,0 +1,21 @@
export const sfViewState = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need those view states. The initial state is already somewhere in this application. We get new view state from the layer when the layer is selected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed view-states.ts

@unclFedor unclFedor closed this Dec 4, 2023
@unclFedor unclFedor deleted the add_arcgis_maplibre_google2 branch December 4, 2023 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@belom88 belom88 belom88 requested changes

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

Successfully merging this pull request may close these issues.
2 participants
@unclFedor @belom88