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.

Sign up for GitHub

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

Jump to bottom

Geomap: Improve location editor #58017

Merged
merged 22 commits into from Nov 22, 2022
Merged

Geomap: Improve location editor #58017

merged 22 commits into from Nov 22, 2022

Conversation

drew08t
Copy link
Contributor

@drew08t drew08t commented Nov 1, 2022 •
edited

What this PR does / why we need it:
When a user is using the location editor, when data source is malformed or missing (or even working, with auto) it is not obvious what is going on. This takes the first steps to improve transparency for the user when using the location editor.

  • Apply filter to add location call
  • create custom component
  • validation logic
  • render warning
  • improve styling
  • include auto working
  • when selecting fields from another query (B for example), if no filter is applied, don't just look at first dataset to address later Geomap: Apply data filter to location editor field selectors #59159
  • only use alert on error, default styling - for success, use subtle text only
  • change location mode to dropdown
  • for field selector error (like no numeric found) use alert under location current error is adequate given new flow
  • add wkt Geomap: Add WKT #58606
  • fix tests
  • when auto is NOT selected, but fields are automatically selected, populate placeholder with field name auto during manual has been disabled for a more intuitive flow (
    image

Current alert styling:
Nov-22-2022 09-53-03

Closes #54404

@drew08t drew08t added this to the 9.3.0 milestone Nov 1, 2022
@drew08t drew08t self-assigned this Nov 1, 2022
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/40251

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/40507

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

⚠️   Possible breaking changes

(Open the links below in a new tab to go to the correct steps)

grafana-data has possible breaking changes (more info)
grafana-ui has possible breaking changes (more info)

Console output
Read our guideline

@grafanabot grafanabot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Nov 3, 2022
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/40830

@grafanabot grafanabot removed the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Nov 4, 2022
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/41069

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/41075

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/41616

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/41632

nmarrs
nmarrs reviewed Nov 22, 2022
View reviewed changes
Copy link
Contributor

@nmarrs nmarrs left a comment

Choose a reason for hiding this comment

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

I think this is ready to be promoted from a draft PR 🎉

public/app/features/transformers/spatial/SpatialTransformerEditor.tsx
@@ -122,7 +122,8 @@ export const SetGeometryTransformerEditor: React.FC<TransformerUIProps<SpatialTr
props.onChange({ ...opts, ...props.options });
console.log('geometry useEffect', opts);
}
});
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ryantxu is useEffect ok to run just once (on initialization of spatial transformer editor)?

public/app/features/geo/editor/locationModeEditor.tsx Outdated Show resolved Hide resolved
public/app/features/geo/editor/locationModeEditor.tsx
if (info) {
if (info.warning) {
return (
<Alert
Copy link
Contributor

Choose a reason for hiding this comment

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

This alert seems pretty large relative to the rest of the editor options

Potentially we can make it a little smaller and add some margin to the top? Also did we want to use the info severity vs warning or error?
Screenshot 2022-11-21 at 5 22 13 PM

Copy link
Contributor Author

@drew08t drew08t Nov 22, 2022 •
edited

Choose a reason for hiding this comment

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

Brought back the old styling I had previously and changed severity to warning:

Nov-22-2022 09-53-03

Only issue I see is that it doesn't fill up the full horizontal span.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More information on the span issue. If we apply width:100%; to the alert's parent div, it will span properly.

image

@drew08t drew08t marked this pull request as ready for review November 22, 2022 18:00
@drew08t drew08t requested a review from a team November 22, 2022 18:00
@drew08t drew08t changed the title [WIP] Geomap: Improve location editor UX and transparency Geomap: Improve location editor UX and transparency Nov 22, 2022
@drew08t drew08t added this to the 9.3.0 milestone Nov 22, 2022
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/42940

@nmarrs nmarrs changed the title Geomap: Improve location editor UX and transparency Geomap: Improve location editor Nov 22, 2022
nmarrs
nmarrs approved these changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@nmarrs nmarrs left a comment

Choose a reason for hiding this comment

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

LGTM

@drew08t drew08t merged commit ee8f292 into main Nov 22, 2022
@drew08t drew08t deleted the geomap-location-editor-feedback branch November 22, 2022 19:24
@nmarrs nmarrs added backport v9.3.x and removed no-backport Skip backport of PR labels Nov 23, 2022
@grafanabot grafanabot mentioned this pull request Nov 23, 2022
Merged
grafanabot pushed a commit that referenced this pull request Nov 23, 2022
@drew08t @grafanabot
Geomap: Improve location editor (#58017)
28966f1 
* add custom component for location editor

* FC cleanup

* Apply filter to add location fields call

* Create custom editor for location mode

* Apply validation logic and render warning

* Improve alert styling

* Add help url button to location alert

* Add success alert for auto

* Remove completed TODOs

* Only use alert on error, not success

* Change location mode to dropdown

* Change alert severity to less severe, info

* Prevent auto field selection during manual

* Update location testing to be for auto mode

* Run geo transformer editor init once

* Fix breaking test

* Clean up some anys

* Update styling for alert

* Remove auto success styling

Co-authored-by: Ryan McKinley <ryantxu@gmail.com>
Co-authored-by: nmarrs <nathanielmarrs@gmail.com>
(cherry picked from commit ee8f292)
nmarrs pushed a commit that referenced this pull request Nov 23, 2022
@grafanabot @drew08t
[v9.3.x] Geomap: Improve location editor (#59170)
280e796 
Geomap: Improve location editor (#58017)

* add custom component for location editor

* FC cleanup

* Apply filter to add location fields call

* Create custom editor for location mode

* Apply validation logic and render warning

* Improve alert styling

* Add help url button to location alert

* Add success alert for auto

* Remove completed TODOs

* Only use alert on error, not success

* Change location mode to dropdown

* Change alert severity to less severe, info

* Prevent auto field selection during manual

* Update location testing to be for auto mode

* Run geo transformer editor init once

* Fix breaking test

* Clean up some anys

* Update styling for alert

* Remove auto success styling

Co-authored-by: Ryan McKinley <ryantxu@gmail.com>
Co-authored-by: nmarrs <nathanielmarrs@gmail.com>
(cherry picked from commit ee8f292)

Co-authored-by: Drew Slobodnjak <60050885+drew08t@users.noreply.github.com>
GuYounes pushed a commit to paul-wurth/BIXpert that referenced this pull request Feb 8, 2023
@grafanabot @drew08t @GuaYounesPW
[v9.3.x] Geomap: Improve location editor (grafana#59170)
73ce1ea 
Geomap: Improve location editor (grafana#58017)

* add custom component for location editor

* FC cleanup

* Apply filter to add location fields call

* Create custom editor for location mode

* Apply validation logic and render warning

* Improve alert styling

* Add help url button to location alert

* Add success alert for auto

* Remove completed TODOs

* Only use alert on error, not success

* Change location mode to dropdown

* Change alert severity to less severe, info

* Prevent auto field selection during manual

* Update location testing to be for auto mode

* Run geo transformer editor init once

* Fix breaking test

* Clean up some anys

* Update styling for alert

* Remove auto success styling

Co-authored-by: Ryan McKinley <ryantxu@gmail.com>
Co-authored-by: nmarrs <nathanielmarrs@gmail.com>
(cherry picked from commit ee8f292)

Co-authored-by: Drew Slobodnjak <60050885+drew08t@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nmarrs nmarrs nmarrs approved these changes

@joshhunt joshhunt Awaiting requested review from joshhunt

Assignees

@drew08t drew08t

Labels
add to changelog area/frontend area/panel/geomap backport v9.3.x enterprise-failed
Projects
None yet
Milestone
9.3.0
Development

Successfully merging this pull request may close these issues.

Geomap: Improve location editor
5 participants
@drew08t @grafanabot @nmarrs @leandro-deveikis @ryantxu