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

refactor(partition): improving on chart events and callbacks #991

Merged
merged 15 commits into from
Jan 28, 2021

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Jan 25, 2021

Summary

This PR

  • improves a bit on the way we access various properties of hierarchical nodes via callbacks we provide
  • replaces a few anys and inferred types
  • doesn't break the API in this regard, but adds to the API and prepares for some deprecation (glad to assist partition users in the change)
  • adds more details for mouse events on the hierarchical data in partitioning, eg. path, depth
  • might push a commit that adds functionality
  • shared at this WIP stage to solicitate feedback
  • not complete; I've some further simplification plan, subject to discussion

Checklist

Delete any items that are not applicable to this PR.

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility
  • Unit tests were updated or added to match the most common scenarios

Sorry, something went wrong.

@monfera monfera added wip work in progress :partition Partition/PieChart/Donut/Sunburst/Treemap chart related API Changes the external API types labels Jan 25, 2021
* - replace users' use of `s.parent` with `s[MODEL_KEY]` for the ShapeTreeNode -> ArrayNode access
* - change MODEL_KEY to something other than 'parent' when it's done (might still be breaking change)
*/
export const MODEL_KEY = 'whatever';
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 specific string "whatever" is of course just for while it's WIP; it'll be parent in the final form of this PR for API compat, but then the usage of .parent can/should be deprecated, to allow us to eventually rename it to eg. model, or even, provide direct access to the model props in the callback functions so this internal representation detail doesn't leak out (my preference, for flatness, not sure if in this PR or in the future)

{
groupByRollup: 'b',
value: 2,
depth: 1,
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 props for the event callback!

@@ -35,6 +35,10 @@ export const Example = () => {
legendStrategy={LegendStrategy.PathWithDescendants}
legendMaxDepth={maxDepth}
theme={STORYBOOK_LIGHT_THEME}
onElementClick={(e) => {
// eslint-disable-next-line no-console
console.log(e);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

WIP, to see the change in playground

@codecov-io
Copy link

codecov-io commented Jan 25, 2021

Codecov Report

Merging #991 (3daca1a) into master (22275cd) will decrease coverage by 0.05%.
The diff coverage is 76.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #991      +/-   ##
==========================================
- Coverage   72.63%   72.57%   -0.06%     
==========================================
  Files         350      366      +16     
  Lines       11050    10669     -381     
  Branches     2433     2197     -236     
==========================================
- Hits         8026     7743     -283     
+ Misses       3010     2828     -182     
- Partials       14       98      +84     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
..._types/goal_chart/state/selectors/picked_shapes.ts 23.52% <ø> (+3.52%) ⬆️
src/specs/settings.tsx 90.00% <ø> (-0.33%) ⬇️
src/state/actions/legend.ts 100.00% <ø> (ø)
...hart_types/partition_chart/layout/config/config.ts 60.00% <20.00%> (-3.89%) ⬇️
...ypes/partition_chart/renderer/canvas/partition.tsx 33.33% <50.00%> (+1.38%) ⬆️
.../chart_types/partition_chart/state/chart_state.tsx 71.18% <66.66%> (-0.49%) ⬇️
.../chart_types/partition_chart/layout/types/types.ts 100.00% <100.00%> (ø)
...es/partition_chart/layout/types/viewmodel_types.ts 85.71% <100.00%> (+2.38%) ⬆️
...es/partition_chart/layout/utils/group_by_rollup.ts 87.62% <100.00%> (+0.26%) ⬆️
...ypes/partition_chart/layout/viewmodel/viewmodel.ts 81.94% <100.00%> (-1.29%) ⬇️
... and 218 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22275cd...3daca1a. Read the comment docs.

@@ -159,7 +159,7 @@ export interface ShapeTreeNode extends TreeNode, SectorGeomSpecY {
path: LegendPath;
dataName: DataName;
value: number;
parent: ArrayNode;
[MODEL_KEY]: ArrayNode;
Copy link
Collaborator

@nickofthyme nickofthyme Jan 25, 2021

Choose a reason for hiding this comment

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

That seems fine to me. Are you trying to avoid breaking changes? I'd say this would be breaking.

One way to somewhat deafen a breaking change would be to keep the parent key but add the @deprecated flag. Replace all usage in kibana with the MODEL_KEY string. Then later we could change the name but it would be a cleaner breaking change when we remove parent.

Suggested change
[MODEL_KEY]: ArrayNode;
/** @deprecated use `MODEL_KEY` instead */
parent: ArrayNode;
[MODEL_KEY]: ArrayNode;

Copy link
Contributor Author

@monfera monfera Jan 25, 2021

Choose a reason for hiding this comment

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

Thanks Nick! The plan would be:

Full compatibility stage, no breaking API, eg. after merging this PR ("whatever" changed back to "parent")

  • we still call it parent
  • everything still works and compliant with our API
  • a new value appears in the API, MODEL_KEY

Deprecation stage:

  • we and users can start replace .parent with [MODEL_KEY]
  • still compatible with the API as is; something gets a @deprecated tag, at least conceptually, as I think we can't have two identically named props at the same time, though haven't tried

Breaking change:

  • When nobody in at least Kibana uses .parent for the model, we make the breaking change of const MODEL_KEY = 'model'
  • though it's a breaking change, it'll no longer impact known users due to our assist during deprecation, while other users simply need to deal with a changing API

I'm not sure if the proposed commit would still work, pls. indicate if that's the case, glad to try. At least from the diff, it's not clear what the deprecation line belongs to

@monfera monfera requested a review from nickofthyme January 25, 2021 20:44
@monfera monfera removed the wip work in progress label Jan 26, 2021
@monfera
Copy link
Contributor Author

monfera commented Jan 26, 2021

Cherrypicked commit e1bef27 is just tighter types, a rename and solving linter warnings with class methods that don't use this. I guess the logic is that it should only be a function if it's a class method; otherwise an arrow function is clearer as arrow functions have no access to this. Probably we keep these as methods rather than static methods or just plain functions for future extensibility, as well as unity among chart types, some of which might use this.

Copy link
Collaborator

@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.

LGTM, provided the class methods are changed to non-fat arrow functions. 👍🏼

@@ -60,6 +60,7 @@ interface MapNode extends NodeDescriptor {
/** @internal */
export const HIERARCHY_ROOT_KEY: Key = '__root_key__';

/** @public */
export type PrimitiveValue = string | number | null; // there could be more but sufficient for now
type Key = CategoryKey;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a huge fan of redefining types like this. But can be changed later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, let's chat

@monfera monfera merged commit d78d1ce into elastic:master Jan 28, 2021
@markov00
Copy link
Member

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes the external API types :partition Partition/PieChart/Donut/Sunburst/Treemap chart related released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants