Limit max number of layers #216
Conversation
|
Need we add a warning message when user reached the max number? |
Codecov Report
@@ Coverage Diff @@
## main #216 +/- ##
==========================================
+ Coverage 87.42% 87.50% +0.07%
==========================================
Files 6 6
Lines 175 176 +1
Branches 24 24
==========================================
+ Hits 153 154 +1
Misses 16 16
Partials 6 6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
common/index.ts
Outdated
| export const MIN_LONGITUDE = -180; | ||
| export const MAP_LAYER_DEFAULT_OPACITY_STEP = 1; | ||
| export const MAP_REFERENCE_LAYER_DEFAULT_OPACITY = 100; | ||
| export const MAX_LAYER_LIMIT = 100; // Move this setting to Map configuration later |
There was a problem hiding this comment.
This setting of 100 is too big for the UI. I would recommend making a default setting as 20(no great reason to support this except that it is small and will make sure that UI is responsive.)
Also, given that whatever value we choose we may endup debating what is the right default value. I would say let's get this value from config file so that customer have a way to update this value if required.
There was a problem hiding this comment.
Agree. In future this value will be part of Map settings for users to configure.
There was a problem hiding this comment.
Why we are not making this setting as of now and putting this as a part of future work?
There was a problem hiding this comment.
wait there is a gap in the understanding. Let me rephrase what I was saying: I was saying we can take this settings from opensearch_dashboards.yml file.
There was a problem hiding this comment.
I don't think we need this to be part of osd yaml file. I am thinking about adding as ui settings similar to other dashboard objects. This will allow users to change this inside Dashboards instead of updating in yaml file, which requires OSD restart i believe
There was a problem hiding this comment.
I agree that it will require an OSD bootstrap and making it as a dynamic settings that can be updated using UI will be great. Please point me to the github issue which we are using to track you proposed approach.
| export const MAP_LAYER_DEFAULT_OPACITY_STEP = 1; | ||
| export const MAP_REFERENCE_LAYER_DEFAULT_OPACITY = 100; | ||
| export const MAX_LAYER_LIMIT = 100; // Move this setting to Map configuration later | ||
| export const MAX_LAYER_NAME_LIMIT = 35; |
There was a problem hiding this comment.
Please add comments why we are using a default value. Your comment can say that this is default value for other components of dashboard will also be fine.
There was a problem hiding this comment.
Do you want comment for layer name or layer limit?
| @@ -108,7 +110,7 @@ export const AddLayerPanel = ({ | |||
| fill | |||
| onClick={showModal} | |||
| aria-label="Add layer" | |||
| isDisabled={IsLayerConfigVisible} | |||
| isDisabled={IsLayerConfigVisible || layerCount >= MAX_LAYER_LIMIT} | |||
There was a problem hiding this comment.
when the add layer button is disabled, I am not able to see the tool-tip which provides info around why this button is disabled. If it is not added, please add the tool-tip, otherwise please refer the code here for the tool tip.
There was a problem hiding this comment.
Added. I was waiting for text review .
VijayanB
force-pushed
the
limit-max-layers
branch
from
34fb649 to
346a71a
Compare
Add restriction to create more than 100 layers. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
VijayanB
force-pushed
the
limit-max-layers
branch
from
346a71a to
d35b5ec
Compare
Add restriction to create more than 100 layers. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> (cherry picked from commit d4a051c)
Description
Add restriction to create more than 100 layers.
Signed-off-by: Vijayan Balasubramanian balasvij@amazon.com
I took snapshot with 3 as max limit to avoid creating 20 layers
Issues Resolved
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.