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

Enable users to select custom vector map for visualization #1718

Merged

Conversation

Shivamdhar
Copy link
Contributor

@Shivamdhar Shivamdhar commented Jun 10, 2022

Description

In case custom maps are uploaded by the user, the layer options tab provides a choice for users to either select default or custom map for visualization. The PR adds logic for custom map selection and projection via leaflet.

Fig. showing choice of selection between custom and default vector maps (in case custom map is uploaded by user)

Screen Shot 2022-06-10 at 11 52 22 AM

Fig. showing us counties visualized on base map (based on custom vector map selected)
Screen Shot 2022-06-10 at 11 54 57 AM

Issues Resolved

#1408
Task - Allow users to select index as custom vector map for visualization

Check List

  • New functionality includes testing. (more tests will be added)
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@Shivamdhar Shivamdhar marked this pull request as ready for review June 10, 2022 19:35
@Shivamdhar Shivamdhar requested a review from a team as a code owner June 10, 2022 19:35
@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2022

Codecov Report

Merging #1718 (903125f) into main (4e117c3) will decrease coverage by 0.00%.
The diff coverage is 57.69%.

@@            Coverage Diff             @@
##             main    #1718      +/-   ##
==========================================
- Coverage   67.52%   67.51%   -0.01%     
==========================================
  Files        3067     3073       +6     
  Lines       58973    59067      +94     
  Branches     8944     8961      +17     
==========================================
+ Hits        39822    39880      +58     
- Misses      16972    17004      +32     
- Partials     2179     2183       +4     
Impacted Files Coverage Δ
src/plugins/region_map/public/region_map_fn.js 100.00% <ø> (ø)
...gion_map/public/components/default_map_options.tsx 50.00% <50.00%> (ø)
...egion_map/public/components/map_choice_options.tsx 53.33% <53.33%> (ø)
src/plugins/region_map/common/constants/shared.ts 100.00% <100.00%> (ø)
...egion_map/public/components/region_map_options.tsx 100.00% <100.00%> (ø)
...ins/region_map/public/components/style_options.tsx 100.00% <100.00%> (ø)
...aps_legacy/public/common/opensearch_maps_client.js 0.00% <0.00%> (ø)
...plugins/maps_legacy/public/map/service_settings.js 1.62% <0.00%> (+0.72%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e117c3...903125f. Read the comment docs.

@ashwin-pc
Copy link
Member

Can we also add some tests for all the new code introduced?

@Shivamdhar Shivamdhar requested a review from ashwin-pc June 15, 2022 17:28
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Thanks Shivam! LGTM

describe('map_choice_options', () => {
it('renders the MapChoiceOptions based on the props provided', async () => {
const props = jest.mock;
const vis = {
Copy link
Member

Choose a reason for hiding this comment

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

nit: since most of the fields here remain the same across the tests, you can set a default one and only override the props you need to be different for each test so that reading the tests is a lot easier.

e.g.

const DEFAULT = {
      colorSchema: {},
      outlineWeight: {},
      ...
}

// And in test 1

const viz = {
    ...DEFAULT,
    ...{
        customProp: something
    }
}

The same comment goes for the other tests too.

Copy link
Member

@VijayanB VijayanB left a comment

Choose a reason for hiding this comment

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

LGTM!

@junqiu-lei junqiu-lei merged commit dfbfec4 into opensearch-project:main Jun 16, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 8, 2022
enable users to select custom vector map for visualization

Signed-off-by: Shivam Dhar <[email protected]>
(cherry picked from commit dfbfec4)
junqiu-lei pushed a commit that referenced this pull request Jul 22, 2022
…1865)

enable users to select custom vector map for visualization

Signed-off-by: Shivam Dhar <[email protected]>
pjfitzgibbons pushed a commit to pjfitzgibbons/OpenSearch-Dashboards that referenced this pull request Dec 1, 2022
…h-project#1718) (opensearch-project#1865)

enable users to select custom vector map for visualization

Signed-off-by: Shivam Dhar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants