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 map page and add basic layers panel #40

Merged
merged 1 commit into from
Sep 8, 2022
Merged

add map page and add basic layers panel #40

merged 1 commit into from
Sep 8, 2022

Conversation

junqiu-lei
Copy link
Member

@junqiu-lei junqiu-lei commented Sep 1, 2022

Signed-off-by: Junqiu Lei [email protected]

Description

  • Structure create map page and list map page
  • Use Maplibre to show base map for development
  • Update react route
    • for maps list: app/maps_dashboards/
    • for map create page: app/maps_dashboards/create
  • Add basic layers panel

Issue

#44

Screenshot

image

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@junqiu-lei junqiu-lei requested a review from a team September 1, 2022 16:47
Copy link
Collaborator

@navneet1v navneet1v left a comment

Choose a reason for hiding this comment

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

Please make a change in the repo so what even when we are raising the PR which gets merged against a feature branch, build check and other checks are run. This will ensure that those checks are not run when we merge the feature branch in the main branch.

@navneet1v
Copy link
Collaborator

The alignment of the Layers and Roadmap in the screenshot seems to be not correct. Please fix that.

@navneet1v
Copy link
Collaborator

Also can we add the UX designer on this PR so that he can take a look at the screen shot only. So that there are no obvious alignment and other issues.

@junqiu-lei
Copy link
Member Author

The alignment of the Layers and Roadmap in the screenshot seems to be not correct. Please fix that.

The commits I pushed is for the basic functions, will follow up UX design and update the PR.

@junqiu-lei
Copy link
Member Author

Please make a change in the repo so what even when we are raising the PR which gets merged against a feature branch, build check and other checks are run. This will ensure that those checks are not run when we merge the feature branch in the main branch.

Sure, since it's new plugin, currently there seems no check workflow related to this feature branch, will add some.

@junqiu-lei
Copy link
Member Author

Also can we add the UX designer on this PR so that he can take a look at the screen shot only. So that there are no obvious alignment and other issues.

Yes, we need verify on the UI. Currently this PR are more focus on the basic functions, will ask UX designer for review once we have the final UI component as designed.

@junqiu-lei junqiu-lei requested review from Shivamdhar, navneet1v and a team September 6, 2022 23:21
Copy link
Contributor

@Shivamdhar Shivamdhar left a comment

Choose a reason for hiding this comment

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

Looks good 🎉

Comment on lines +10 to +11
export const MAP_VECTOR_TILE_URL =
'https://dldbnqfps17cd.cloudfront.net/styles/basic-preview/compressedstyle.json';
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we cannot replace it right now? If there is a tracking GH issue please attach a TODO here

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the production vector tiles service uses OpenStreetMap, which is under review by legal team, will update then.

Comment on lines 13 to 22
export const MAP_INITIAL_STATE = {
lng: 11,
lat: 49,
zoom: 2,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

please specify what these coordinates represents.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will update to

  lng: 0,
  lat: 0,
  zoom: 1,

</>
<div>
<Switch>
<Route exact path="/create-map" render={(props) => <Map />} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we make constants for these path names?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

*/

@import "maplibre-gl/dist/maplibre-gl.css";

/* stylelint-disable no-empty-source */
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can remove this.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need remain it for Maplibre right format https://maplibre.org/maplibre-gl-js-docs/api/#maplibre-css.

@junqiu-lei junqiu-lei merged commit 97b9b3a into opensearch-project:feature/new-maps Sep 8, 2022
@junqiu-lei junqiu-lei added feature v2.5.0 'Issues and PRs related to version v2.5.0' labels Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature maps-dashboards v2.5.0 'Issues and PRs related to version v2.5.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants