-
Notifications
You must be signed in to change notification settings - Fork 20
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
Ability to select order (Desc/Asc) for "raw data" metrics aggregations #88
Conversation
@lvta0909 sorry for the delay in reviewing, this looks really good to me! I merged in master and made some minor changes if you'd like to take a look! |
@iwysiu mind reviewing this since I added a few more commits? I can merge/release once you think it looks good. |
Also I believe the build is failing because the levity check fails for all external contributors right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const newOrderValue = 'asc'; | ||
await userEvent.click(screen.getByText('Ascending')); | ||
expect(onChange).toHaveBeenCalledTimes(2); | ||
// expect(onChange).toHaveBeenCalledWith(expect.objectContaining({'order' : 'asc'})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be removed/uncommented?
@@ -28,6 +28,8 @@ interface Props { | |||
previousMetrics: MetricAggregation[]; | |||
} | |||
|
|||
const orderLabel = 'Test Order select Container'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to be a separate variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this altogether, since afaik InlineField doesn't need an explicit arial-label
@@ -256,3 +258,9 @@ export const getChildren = (metric: MetricAggregation, metrics: MetricAggregatio | |||
|
|||
return [...children, ...children.flatMap(child => getChildren(child, metrics))]; | |||
}; | |||
|
|||
// TODO: Define better types for the following | |||
export const orderOptions = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const orderOptions = [ | |
export const orderOptions: Array<SelectableValue<string>> = [ |
What?
Add ability to select the query order for "raw data" metric agreggation.
Why?
Sometimes it is needed to query the firsts X documents starting from a defined time. With the default query order as Descending it was not directly possible as it would give the lasts X documents from the ending time.
The ability to choose the order solves this limitation.
This functionality is available only to "raw data" and "raw document" metric queries.