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

[Canvas] XY. Step 1. Remove all specific logic, related to aggType. #111876

Merged
merged 33 commits into from
Sep 22, 2021

Conversation

Kuznietsov
Copy link
Contributor

Completes part of #110430 and #101377

Step 1. XY. Remove all specific logic, related to aggType completed.

@Kuznietsov Kuznietsov added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:large Large Level of Effort v8.0.0 release_note:skip Skip the PR/issue when compiling release notes impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Feature:Canvas auto-backport Deprecated - use backport:version if exact versions are needed labels Sep 10, 2021
@Kuznietsov Kuznietsov requested a review from alexwizp September 10, 2021 15:35
@Kuznietsov Kuznietsov self-assigned this Sep 10, 2021
@Kuznietsov Kuznietsov requested a review from a team as a code owner September 10, 2021 15:35
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov Kuznietsov changed the base branch from presentation/feature-xy to master September 13, 2021 06:12
# Conflicts:
#	src/plugins/vis_types/xy/kibana.json
#	src/plugins/vis_types/xy/public/index.ts
#	src/plugins/vis_types/xy/public/utils/accessors.tsx
@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov Kuznietsov changed the base branch from master to presentation/feature-xy September 14, 2021 15:00
@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. Tested various edge cases on bar charts for ranges with complex formatters.

@Kuznietsov
Copy link
Contributor Author

@stratoula and @markov00, could you, please, review the current PR ) thanks for your efforts.

@Kuznietsov
Copy link
Contributor Author

@nickofthyme, thanks for your review)

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

# Conflicts:
#	src/plugins/vis_types/xy/public/services.ts
#	src/plugins/vis_types/xy/public/utils/accessors.tsx
#	src/plugins/vis_types/xy/public/vis_component.tsx
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Code LGTM, I tested various aggs and combinations and it seems to work fine. I just left two questions :)

As Nick has already approved I think we are fine but if anyone else would like to play it - especially with the bwc - feel free to also test it my dear @elastic/kibana-vis-editors team

const tickFormatter =
aggType === BUCKET_TYPES.DATE_RANGE || aggType === BUCKET_TYPES.RANGE ? identity : formatter;

const tickFormatter = (v: any) =>
Copy link
Contributor

@stratoula stratoula Sep 21, 2021

Choose a reason for hiding this comment

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

Just checking here. Can we omit using any here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done)

@@ -278,7 +297,9 @@ export const visTypeXyVisFn = (): VisTypeXyExpressionFunctionDefinition => ({
splitRow: args.splitRowDimension,
splitColumn: args.splitColumnDimension,
},
} as VisParams;
// ------------------------------------------------------------------------------------------------------------------
} as VisParams; /* @TODO: rewrite this `as VisParams` to real `VisParams` via changing accessor
Copy link
Contributor

Choose a reason for hiding this comment

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

All these TODOs are going to be handled in the next steps?

Copy link
Contributor Author

@Kuznietsov Kuznietsov Sep 21, 2021

Choose a reason for hiding this comment

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

as for real, I can remove those Todos , because I've completed mostly all of them )

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @Kunzetsov

@Kuznietsov Kuznietsov merged commit a35b18a into elastic:presentation/feature-xy Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Canvas impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants