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

Improve UX #142

Merged
merged 5 commits into from
Dec 30, 2022
Merged

Conversation

VijayanB
Copy link
Member

@VijayanB VijayanB commented Dec 23, 2022

Description

  1. Set new layer name as New layer
  2. Set default OpenSearch Map as "Default map"
  3. Added tooltip for buttons
  4. Layer type icon is added before title

Demo
Icons are updated as below and updated layer names
Screen Shot 2022-12-27 at 4 09 38 PM

Add layer panel

Screen Shot 2022-12-27 at 4 37 06 PM

Layer type Icon is added before tile
Screen Shot 2022-12-22 at 11 38 13 PM

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.

@VijayanB VijayanB requested a review from a team December 23, 2022 03:07
@VijayanB VijayanB self-assigned this Dec 23, 2022
@VijayanB VijayanB force-pushed the improve-ux branch 5 times, most recently from 8bacaad to 32f8002 Compare December 23, 2022 07:42
@@ -301,7 +301,7 @@ export const LayerControlPanel = memo(
<EuiFlexItem>
<EuiListGroupItem
key={layer.id}
label={layer.name}
label={layer.name || layersTypeNameMap[layer.type]}
Copy link
Member

Choose a reason for hiding this comment

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

For pending new layer, how about name "New " + layersTypeNameMap[layer.type]

Copy link
Member

Choose a reason for hiding this comment

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

Synced with Vijay, let's have input from UX 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.

Updated new layer name as New layer

junqiu-lei
junqiu-lei previously approved these changes Dec 23, 2022
@VijayanB VijayanB changed the title Set default name as empty for new layer and disable tooltip Improve UX Dec 28, 2022
@VijayanB VijayanB requested a review from junqiu-lei December 28, 2022 00:29
@VijayanB VijayanB force-pushed the improve-ux branch 2 times, most recently from dc12ace to d9eff11 Compare December 28, 2022 00:38

function onClickAddNewLayer(layerType: string) {
const initLayerConfig = getLayerConfigMap()[layerType];
initLayerConfig.name = 'New layer ' + nextLayerIndex;
Copy link
Member

Choose a reason for hiding this comment

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

How about use layers.length + 1 for the new layer number? The nextLayerIndex will keep increasing even if I create a new layer but discard it.

Copy link
Member Author

@VijayanB VijayanB Dec 28, 2022

Choose a reason for hiding this comment

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

Sure. But i don't think we have access to total layer inside Add layer panel, if not, how to get layer count without adding new dependency? or else, i have to move inside layer_control_panel. Let me see how does it look if i move it inside add_layer()

Copy link
Member

@junqiu-lei junqiu-lei Dec 28, 2022

Choose a reason for hiding this comment

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

We can create a function in LayerControlPanel and pass it to AddLayerPanel which can help get new layer index from layers.

  const getNewLayerIndex = () => {
    return layers.length + 1;
  };

Copy link
Member

Choose a reason for hiding this comment

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

I got error message from initLayerConfig.name = 'New layer ' + nextLayerIndex;, we might need to change not use enum for DASHBOARDS_MAPS_LAYER_NAME
TS2322: Type 'string' is not assignable to type 'DASHBOARDS_MAPS_LAYER_NAME'.

Copy link
Member Author

Choose a reason for hiding this comment

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

kept default value as empty string.

@@ -4,7 +4,7 @@
*/

import { Map as Maplibre } from 'maplibre-gl';
import { DASHBOARDS_MAPS_LAYER_NAME, DASHBOARDS_MAPS_LAYER_TYPE } from '../../common';
import { DASHBOARDS_MAPS_LAYER_ICON, DASHBOARDS_MAPS_LAYER_NAME, DASHBOARDS_MAPS_LAYER_TYPE } from "../../common";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { DASHBOARDS_MAPS_LAYER_ICON, DASHBOARDS_MAPS_LAYER_NAME, DASHBOARDS_MAPS_LAYER_TYPE } from "../../common";
import {
DASHBOARDS_MAPS_LAYER_ICON,
DASHBOARDS_MAPS_LAYER_NAME,
DASHBOARDS_MAPS_LAYER_TYPE,
} from '../../common';

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

@@ -153,7 +156,7 @@ const TooltipTable = ({
</EuiFlexGroup>
<EuiSpacer size="s" />
<EuiFlexGroup justifyContent="spaceAround" alignItems="center" gutterSize="none">
{showLayerSelection && (
{showLayerSelection && options?.length > 1 && (
Copy link
Member

Choose a reason for hiding this comment

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

Should this be >=1?

Copy link
Member Author

@VijayanB VijayanB Dec 28, 2022

Choose a reason for hiding this comment

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

I think we need layer selection only if there are more than 1 layer.

@VijayanB VijayanB requested a review from vamshin December 28, 2022 01:45
@VijayanB VijayanB force-pushed the improve-ux branch 2 times, most recently from 298c475 to d95af64 Compare December 28, 2022 02:48
@junqiu-lei
Copy link
Member

Shall we also update the layer name to "Default map" at add new layer modal?

@VijayanB
Copy link
Member Author

VijayanB commented Dec 28, 2022

Shall we also update the layer name to "Default map" at add new layer modal?

Default map is layer name for default layer created by Maps. We don't have to rename OpenSearch Map as default map since in future we might allow users to customize this default layer.

junqiu-lei
junqiu-lei previously approved these changes Dec 29, 2022
junqiu-lei
junqiu-lei previously approved these changes Dec 29, 2022
Signed-off-by: Vijayan Balasubramanian <[email protected]>
Set new layer name as New layer
Set default OpenSearch Map as "Default map"
Added tooltip for buttons
Layer type icon is added before title

Signed-off-by: Vijayan Balasubramanian <[email protected]>
Show layer selection only if more than 1
layers are available.

Signed-off-by: Vijayan Balasubramanian <[email protected]>
Signed-off-by: Vijayan Balasubramanian <[email protected]>
Signed-off-by: Vijayan Balasubramanian <[email protected]>
@VijayanB VijayanB merged commit ccfe171 into opensearch-project:feature/new-maps Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants