Skip to content

fix(discover2): Properly sync minigraphs of pre-built queries with the global selection header#15868

Merged
dashed merged 7 commits into
masterfrom
discover2-fix-minigraph-sync-with-global-selection-header
Dec 2, 2019
Merged

fix(discover2): Properly sync minigraphs of pre-built queries with the global selection header#15868
dashed merged 7 commits into
masterfrom
discover2-fix-minigraph-sync-with-global-selection-header

Conversation

@dashed

@dashed dashed commented Nov 28, 2019

Copy link
Copy Markdown
Member

This only applies to pre-built queries; saved queries are still disconnected.

I've renamed EventView.fromSavedQueryWithLocation() to EventView. fromNewQueryWithLocation(), and refined some TS types.

TODO

  • remove isLegacySavedQuery()

@dashed dashed self-assigned this Nov 28, 2019
}

static fromSavedQuery(saved: NewQuery | LegacySavedQuery): EventView {
static fromSavedQuery(saved: NewQuery | SavedQuery): EventView {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You could remove the isLegacySavedQuery() conditions too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

});
});

it('new query takes precedence over global selection values', function() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this result in the global header saying one thing but the result set being for a different range? How do we sync those up?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@markstory Good question. Right now, I've meant EventView.fromNewQueryWithLocation() to be only used for pre-built queries; which at the moment, do not have statsPeriod baked in. But I've written code in the case if a decision was made to bake statsPeriod in the upcoming weeks.

This merging strategy is used for the "Build a new query" CTA, and as well as for the query cards CTA for pre-built queries (and their minigraphs).

This is primarily for the queries we display on the landing page; since the global header is accessible to the user on this page. In other words, changing the values on the global header should be reflected to the minigraphs on the prebuilt query cards.

Regarding your concerns in sync'ing of the global header and the query, we already ensure that EventView.fromLocation() is the source of truth for the query builder view.


For saved queries, any values they may have will override the global selection values (this is what EventView.fromSavedQuery() does).

@dashed dashed force-pushed the discover2-fix-minigraph-sync-with-global-selection-header branch from ad5348b to 38231fb Compare November 30, 2019 00:53
@dashed dashed merged commit 5cf8c38 into master Dec 2, 2019
@dashed dashed deleted the discover2-fix-minigraph-sync-with-global-selection-header branch December 2, 2019 14:29
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 19, 2020
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