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

docs: fix filterSeriesInTooltip predicate #552

Merged
merged 2 commits into from
Feb 19, 2020

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Feb 14, 2020

Summary

This PR fix a small bug on storybook (story BarChart - 2y1g) where an example was using a wrong FilterPredicate on the filterSeriesInTooltip props.

I've also took the opportunity of this PR to remove all the getSpecId and getAxisId calls as they are no longer needed

Checklist

Use strikethroughs to remove checklist items you don't feel are 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, including a check against IE11
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • [ ] Unit tests were updated or added to match the most common scenarios
  • Each commit follows the convention

@markov00 markov00 added the :docs Anything related to documentation, API, storybook label Feb 14, 2020
@markov00 markov00 changed the title docs: fix filteredLabel ids docs: fix filterSeriesInTooltip predicate Feb 14, 2020
@codecov-io
Copy link

codecov-io commented Feb 14, 2020

Codecov Report

Merging #552 into master will increase coverage by <.01%.
The diff coverage is 98.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #552      +/-   ##
==========================================
+ Coverage   71.49%   71.49%   +<.01%     
==========================================
  Files         208      208              
  Lines        6118     6119       +1     
  Branches     1169     1169              
==========================================
+ Hits         4374     4375       +1     
  Misses       1727     1727              
  Partials       17       17
Impacted Files Coverage Δ
src/chart_types/xy_chart/utils/interactions.ts 100% <ø> (ø) ⬆️
src/chart_types/partition_chart/specs/index.ts 60% <ø> (ø) ⬆️
.../chart_types/xy_chart/renderer/dom/highlighter.tsx 82.14% <ø> (ø) ⬆️
src/utils/events.ts 100% <ø> (ø) ⬆️
...y_chart/state/selectors/is_tooltip_snap_enabled.ts 100% <ø> (ø) ⬆️
.../selectors/get_tooltip_values_highlighted_geoms.ts 92.42% <ø> (ø) ⬆️
src/utils/accessor.ts 45.45% <ø> (ø) ⬆️
...es/partition_chart/layout/types/viewmodel_types.ts 100% <ø> (ø) ⬆️
...types/partition_chart/layout/types/config_types.ts 100% <ø> (ø) ⬆️
.../chart_types/partition_chart/layout/types/types.ts 100% <ø> (ø) ⬆️
... and 46 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 2543e2c...6273b43. Read the comment docs.

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, finally, has this day actually come where we remove all getXXXId??

Side note: I think we should have a react linting rule in place for string and boolean props, what do you think?

// bad
id={'bottom'}
// good
id="bottom"

// bad
showLegend={true}
// good
showLegend

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.

Can we also remove the function definitions and make this a breaking change with the tooltip logic? That would finish this once and for all.

export function getGroupId(id: string): string {
return id;
}
export function getAxisId(id: string): string {
return id;
}
export function getSpecId(id: string): string {
return id;
}
export function getAnnotationId(id: string): string {
return id;
}
export type GroupId = string;
export type AxisId = string;
export type SpecId = string;
export type AnnotationId = string;

@markov00
Copy link
Member Author

Can we also remove the function definitions and make this a breaking change with the tooltip logic? That would finish this once and for all.

export function getGroupId(id: string): string {
return id;
}
export function getAxisId(id: string): string {
return id;
}
export function getSpecId(id: string): string {
return id;
}
export function getAnnotationId(id: string): string {
return id;
}
export type GroupId = string;
export type AxisId = string;
export type SpecId = string;
export type AnnotationId = string;

@nickofthyme Sure will do, I will open up a PR for this

LGTM, finally, has this day actually come where we remove all getXXXId??

Side note: I think we should have a react linting rule in place for string and boolean props, what do you think?

// bad
id={'bottom'}
// good
id="bottom"

// bad
showLegend={true}
// good
showLegend

@rshen91 do you want to include also this eslint rule on your PR?

@markov00 markov00 merged commit 8262d19 into elastic:master Feb 19, 2020
@markov00 markov00 deleted the 2020-02-13_fix-story-ids branch February 19, 2020 15:55
markov00 added a commit that referenced this pull request Feb 21, 2020
markov00 added a commit that referenced this pull request Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:docs Anything related to documentation, API, storybook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants