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

Fix Map bounding box controls #6863

Closed
wants to merge 5 commits into from

Conversation

panda01
Copy link
Contributor

@panda01 panda01 commented Apr 12, 2016

Changes the push filter to instead also modify if there is already one.
Also properly set the title for visualizations.

Fixes #3731.

@panda01 panda01 force-pushed the fix/map-bounding-box branch 2 times, most recently from ceb3faf to 6b6d15f Compare April 12, 2016 18:19
@panda01 panda01 added the review label Apr 13, 2016
@panda01 panda01 changed the title [WIP] Fix Map bounding box controls Fix Map bounding box controls Apr 13, 2016
@@ -46,6 +46,7 @@ module.exports = function VislibRenderbotFactory(Private) {
VislibRenderbot.prototype.buildChartData = buildChartData;
VislibRenderbot.prototype.render = function (esResponse) {
this.chartData = this.buildChartData(esResponse);
this.chartData.title = this.vis.title || '';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This in my experience has never been undefined, it's usually 'New Visualization' when I would expect that, put the OR for safekeeping.
Added this when I thought the filters were panel specific and started implementing this for that, found out they weren't and left this piece for the future since it shouldn't be undefined anyway.

@rashidkpc
Copy link
Contributor

Currently this overwrites the entire filter object. It actually overwrites all geo filter objects. So couple things:

  • Don't overwrite disabled or pinned filters
  • If the user sets an alias on the filter, preserve the alias

@rashidkpc rashidkpc assigned panda01 and unassigned rashidkpc Apr 14, 2016
@rashidkpc
Copy link
Contributor

Also get rid of that title, it just duplicate's the data shown elsewhere in the UI

@panda01
Copy link
Contributor Author

panda01 commented Apr 15, 2016

I've addressed your concerns @rashidkpc, I couldn't reproduce the overwriting of pinned filters, but everything else has been corrected.

@panda01 panda01 assigned rashidkpc and unassigned panda01 Apr 15, 2016
const isDisabled = filt.meta.disabled;
const isSameFilterType = filt.hasOwnProperty(filterKey);
if (!isDisabled && isSameFilterType) {
_.assign(pendingFilter.meta, filt.meta); // be sure to keep any meta data
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately I don't think you can keep all of the meta key as it links to the index pattern, you'll end up with filters referencing the wrong pattern. You can keep all the rest, but you need to replace the index pattern with the right one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, so should I check and make sure they're the same index pattern?I figured if checking to make sure they're the same doesn't matter, neither does overwriting it.

Also, what do I do in the ui to make it change?

@rashidkpc rashidkpc assigned panda01 and unassigned rashidkpc Apr 15, 2016
@rashidkpc
Copy link
Contributor

@panda01 yeah, my mistake on the pinned filters thing.

@panda01 panda01 assigned rashidkpc and unassigned panda01 Apr 25, 2016
@rashidkpc
Copy link
Contributor

So there's an interesting behavior here that I'm not sure how to handle.

  1. Filter to a section of data
  2. Now select a section of the map you know had data before the filter, but doesn't now.
  3. Data will appear

Feels like a weird interaction, filters usually only filter down. It sort of seems like we should always be fitting the new filter within the bounds of the old one right? Maybe I'm just being pedantic. @tbragin can you help me out here?

@rashidkpc rashidkpc assigned tbragin and unassigned rashidkpc Apr 26, 2016
@tbragin
Copy link
Contributor

tbragin commented May 3, 2016

@rashidkpc kpc @panda01 I checked out the PR and played with the current implementation.

In trying to think of user's expectations, when they add multiple filters to a geomap, there are three scenarios I can imagine:
(1) Successive drill-ins within the same region
(2) Replacing an existing filter with another one, but not always trying to successively drill-in
(3) Trying to select multiple filters, expecting them to add up (union of multiple rectangles)

The current implementation accomplishes (2), Rashid is asking whether we should actually be doing (1), and someone else reading this ticket may wonder if we in fact should be doing (3).

Either way, some minor issues I see with the current approach are:

  • User has to anticipate how replacing one filter with another works -- basically every time you add a rectangle filter, the previous one is invalidated, and as discussed above, they may expect something different, like (3)
  • There is no visual cue that one of the currently selected filters is replaced by another, replacement just happens and in a busy dashboard you may miss that fact
  • There is no indication when the user makes a selection that is not a successive drill-in that there is data underlying that selection. Could we show the data we are currently filtering out as a partially transparent overlay while the user is making another selection? But not sure that's the right approach, this could also be jarring and probably computationally expensive.
  • There is no opportunity for a user to cancel this action. I'm actually ok with that, doing anything else will add extra clicks, but just noting here as not everyone may actually be interested in invalidating the previous filter.

As @rashidkpc notes, we could constrain the behavior and require successive drill-ins, but:

  • This would add extra clicks for any user trying to accomplish (2)
  • We'd have to add visual cues to let the user know about the constraint, i.e. somehow indicate to the user why we are rejecting a filter outside of the current drill-in, rather than just not showing any data silently. For instance:
    • Could we gray out the rest of the map, so it's clear that the only place to drill in is in the region already selected?
    • If someone still tries to select outside this region, could we make the rectangle red, as an indication that the user is doing something wrong?
    • If someone does select another rectangle, should we throw an error?

All in all, i'm probably ok with keeping (2) assuming we make it more obvious to the user what's happening there, and re-evaluate if we get negative feedback. Is there any world in which we would consider doing (3)?

2016-05-03 14_04_27

@tbragin tbragin assigned panda01 and unassigned tbragin May 3, 2016
@panda01
Copy link
Contributor Author

panda01 commented May 5, 2016

Could we show the data we are currently filtering out as a partially transparent overlay while the user is making another selection?

I think that the showing of partially greyed out buckets would be very useful, however it isn't very reliable because that information depends on a request to es which could be plagued with all kind of issues on the wire taking away from the experience.

Is there any world in which we would consider doing (3)?

I would be down for this if this is possible with elasticsearch, in my looking at the docs, i couldn't find a mention of it, and the structure doesn't make it look doable

I do think that we should give some sort of indication to the user that their current action will overwrite what ever filter they have set. Maybe even something as simple as a caution icon next to it, or in place of it on hover, with some tooltip text for help.

@panda01 panda01 assigned tbragin and unassigned panda01 May 5, 2016
@panda01
Copy link
Contributor Author

panda01 commented May 5, 2016

@tbragin @rashidkpc IMO we should put some kind of indicator on or around the filter icon that give information on hover since that is really one of the more viable solutions, should I move forward with that, I'm completely open to better suggestions!

@panda01 panda01 added discuss and removed review labels Jul 5, 2016
@epixa epixa added the help wanted adoptme label Oct 8, 2016
@tbragin tbragin added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Nov 12, 2016
@thomasneirynck
Copy link
Contributor

I agree with @tanya. it is not entirely clear what behaviour we exactly want. There is also the possibility that we want panning/zooming to automatically adjust the filters (cf. #8087).

For those reasons, I'm closing this for now. We're also looking at an overhaul of the map so we can revisit #3731 and this PR in that context.

Ikuni17 pushed a commit that referenced this pull request Jul 6, 2023
`eui@82.1.0` ⏩ `83.0.0`

⚠️ The biggest change in this PR by far is the `EuiButtonEmpty` Emotion
conversion, which changes the DOM structure of the button slightly as
well as several CSS classes around it.

EUI has attempted to convert any custom EuiButtonEmpty CSS overrides
where possible, but would super appreciate it if CODEOWNERS checked
their touched files. If anything other than a snapshot or test was
touched, please double check the display of your button(s) and confirm
everything still looks shipshape. Feel free to ping us for advice if
not.

---

## [`83.0.0`](https://github.com/elastic/eui/tree/v83.0.0)

**Bug fixes**

- Fixed `EuiPaginationButton` styling affected by `EuiButtonEmpty`'s
Emotion conversion ([#6893](elastic/eui#6893))

**Breaking changes**

- Removed `isPlaceholder` prop from `EuiPaginationButton`
([#6893](elastic/eui#6893))

## [`82.2.1`](https://github.com/elastic/eui/tree/v82.2.1)

- Updated supported Node engine versions to allow Node 16, 18 and >=20
([#6884](elastic/eui#6884))

## [`82.2.0`](https://github.com/elastic/eui/tree/v82.2.0)

- Updated EUI's SVG icons library to use latest SVGO v3 optimization
([#6843](elastic/eui#6843))
- Added success color `EuiNotificationBadge`
([#6864](elastic/eui#6864))
- Added `badgeColor` prop to `EuiFilterButton`
([#6864](elastic/eui#6864))
- Updated `EuiBadge` to use CSS-in-JS for named colors instead of inline
styles. Custom colors will still use inline styles.
([#6864](elastic/eui#6864))

**CSS-in-JS conversions**

- Converted `EuiButtonGroup` and `EuiButtonGroupButton` to Emotion
([#6841](elastic/eui#6841))
- Converted `EuiButtonIcon` to Emotion
([#6844](elastic/eui#6844))
- Converted `EuiButtonEmpty` to Emotion
([#6863](elastic/eui#6863))
- Converted `EuiCollapsibleNav` and `EuiCollapsibleNavGroup` to Emotion
([#6865](elastic/eui#6865))
- Removed Sass variables `$euiCollapsibleNavGroupLightBackgroundColor`,
`$euiCollapsibleNavGroupDarkBackgroundColor`, and
`$euiCollapsibleNavGroupDarkHighContrastColor`
([#6865](elastic/eui#6865))

---------

Co-authored-by: Cee Chen <constance.chen@elastic.co>
Co-authored-by: Jeramy Soucy <jeramy.soucy@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:Visualizations Generic visualization features (in case no more specific feature label is available) help wanted adoptme release_note:enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants