Skip to content
This repository has been archived by the owner on Aug 1, 2024. It is now read-only.

89 show multiple regions layers #97

Merged
merged 12 commits into from
Jan 7, 2022
Merged

Conversation

srezacova
Copy link
Collaborator

@srezacova srezacova commented Dec 29, 2021

Description

Show multiple region layers. Create layerOptions in mapLayers config.

How has this been tested?

TEST singleLayer: true with 2 timeseries layers behavior:

    referenceId: 'DailyCovid19Cases',
    date: new Date(now - 1 * oneDay).toISOString(),
    csvFileName: 'daily_cases_test/Daily_Covid19_cases1.csv',
  },
  {
    referenceId: 'DailyCovid19Cases',
    date: new Date(now - 2 * oneDay).toISOString(),
    csvFileName: 'daily_cases_test/Daily_Covid19_cases2.csv',
  },
  {
    referenceId: 'DailyCovid19Cases',
    date: new Date(now - 3 * oneDay).toISOString(),
    csvFileName: 'daily_cases_test/Daily_Covid19_cases3.csv',
  },
  • add test layer to MapLayers.yml
  geoReferenceId: "slovakiaRegions"
  layerType: "regions"
  category: "Baseline data"
  title: "Daily Covid-19"
  attribute: "Daily Covid19 Cases"
  attributeDescription:
    descriptionText: "COVID-19 New Cases: {{Daily Covid19 Cases}}"
    featureText: "{{featureId}} kraj"
  featureId: "name"
  style:
    fillColor:
      type: "colormap"
      value: "green"
    min: 0
    max: 100
    strokeColor:
      type: "color"
      value: "rgba(255,255,255,0.2)"
  legend:
    - type: "colormap"
      color: "green"
      min: 0
      max: 100
  metadata:
    description: "Sample data created just for this purpose. Do not represent the reality in any time."
    updateFrequency: "never"
    unit: "n/a"
    dataRetrievalDescription: "Data was randomly created."
  layerOptions: 
    singleDisplay: false   
    timeseries: true

Leave singleDisplay false for both timeseries layers to check functionality with more sliders shown on the map. Change singleDisplay to true in other layers for checking singleDisplay functionality. Add maxResolution: 1000 to any of the layers to check if it works.

Run ./start.sh, ./runinitialload.sh with COUNTRY=Sample. Check layers on the map.
Now you have 2 timeseries layers for testing purposes, one in Baseline data and one in Covid-19. You can add one more for testing purposes.

  • check points in Show multiple region layers - 3SP #89
  • check new functionality in publicMap.js and if comments are understandable
  • check layer without values, that it is transparent gray
  • run tests in api
  • check release notes
  • check documentation and schema changes
  • slider behavior is the same for all the layer types, singleDisplay behavior is added just for regions layers, other layer types will act like before, so they will appear together with regions layers, singleDisplay flag in them won't have any effect
  • this task doesn't solve layer ordering, when 2 region layers overlay each other, the color doesn't corespond to legend, because the colors are mixed together, we should use z-index later to fix this.
  • check group layer

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have checked to ensure that there aren't other open Pull Requests for the same update/change.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@srezacova
Copy link
Collaborator Author

Hi,
mark 'timeseries' on top level as deprecated in release notes

So I will add this in CHANGELOG.md?

[1.3.1] - 2021-12-29

Deprecated

  • 'timeseries' key on top level is now deprecated, it is moved to 'layerOptions' key

or something like that?

@bariela
Copy link
Collaborator

bariela commented Dec 30, 2021

Hi, mark 'timeseries' on top level as deprecated in release notes

So I will add this in CHANGELOG.md?

[1.3.1] - 2021-12-29

Deprecated

  • 'timeseries' key on top level is now deprecated, it is moved to 'layerOptions' key

or something like that?

in the unreleased part for now, as we don't make new release just for this feature and with more specification, so the reader can understand what and where exactly should be changed

@srezacova
Copy link
Collaborator Author

Hi, mark 'timeseries' on top level as deprecated in release notes
So I will add this in CHANGELOG.md?

[1.3.1] - 2021-12-29

Deprecated

  • 'timeseries' key on top level is now deprecated, it is moved to 'layerOptions' key

or something like that?

in the unreleased part for now, as we don't make new release just for this feature and with more specification, so the reader can understand what and where exactly should be changed

I fixed this in release notes.

@srezacova srezacova closed this Jan 3, 2022
@srezacova srezacova reopened this Jan 3, 2022
@@ -124,6 +124,10 @@ const TimeSeriesSlider = ({ availableDates, modifiedLayer }) => {

const [currentValue, setCurrentValue] = useState(undefined);

useEffect(() => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When you have more layers in map visible with slider and deselect one of these layers, slider stay on the date you selected before on this layer. So I added this useEffect to always show LAST value. You can comment out this and check the behavior or if we should do it another way. In PublicMap.js we have code on LN142-144, if I comment this out, slider behave the same. What was the purpose of it? I suppose this should fetch the LAST values for the layer, but we have LAST values also without it, when you show the slider for the first time when we use it on LN214. (Thats because of LN132-134 in TimeSeriesSlider.js.) And when we use it on LN171 in PublicMap.js, this doesn't set slider to LAST, because currentValue already exist. So my question is should I remove PublicMap.js LN142-144? And leave this code in TimeSeriesSlider? Thanks for checking

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi, so this is ok?

@srezacova srezacova changed the title Draft 89 show multiple regions layers 89 show multiple regions layers Jan 3, 2022
setTimeSeriesSlider(false);
setModifiedLayer(null);
}
} else {
// layer is not selected, selecting
// deselect the rest of regions layers, if regions layer selected
// deselect the rest of regions layers, if regions layer with singleDisplay true is selected
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this comment should be to if on L182

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I moved it.

}
});
} else {
// deselect regions layer, which have singleDisplay true, if regions layer with singleDisplay false is selected
Copy link
Collaborator

Choose a reason for hiding this comment

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

'regions layer, which has'
it's only one ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah you are right :) fixed

@@ -7,8 +7,17 @@ const groupLayer = (layerData, handleIsLoading) => {
title: layerData.title,
type: layerData.layerType,
legend: layerData.legend,
layers: layerData.layers.map((layer) => staticLayerGenerator({ ...layer, visible: true }, handleIsLoading)),
timeseries: layerData.timeseries,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be still compatible with this data structure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed this and tested it. Please check it. I removed 'timeseries' key from documentation (moved it to layerOptions). Should I also leave the documentation as it was before + add layerOptions description?

@srezacova srezacova requested a review from bariela January 6, 2022 21:27
@@ -6,6 +6,11 @@ const logger = require('../config/winston');
const { getOneLayerGeoData, saveMapLayers } = require('./db');

const addMapLayer = async (data) => {
if (data.timeseries !== undefined) {
logger.info(
`DeprecationWarning: 'timeseries' key on the top level is deprecated. Move 'timeseries' key in 'layerOptions' key in ${data.referenceId} layer in mapLayers config.`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@bariela bariela merged commit dca4075 into main Jan 7, 2022
@bariela bariela deleted the 89-show-multiple-regions-layers branch January 7, 2022 13:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants