-
Notifications
You must be signed in to change notification settings - Fork 122
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
feat(annotations): add annotations to DebugState #1434
Conversation
packages/charts/src/chart_types/xy_chart/state/selectors/get_debug_state.ts
Outdated
Show resolved
Hide resolved
packages/charts/src/state/types.ts
Outdated
// API Extractor failing with LineAnnotationDatum | RectAnnotation, the dataValue in get_debug_state can be any value | ||
data?: any; |
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.
can you please describe the API extractor failure?
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 get warnings when changing the type from any that result in the yarn api:check failing with results expected same but got worse but now when Im running it, it's not producing those errors d5591a0 for changes
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.
yep now it looks good no?
can you remove the comment on the code?
// API Extractor failing with LineAnnotationDatum | RectAnnotationDatum, the dataValue in get_debug_state can be any value
packages/charts/src/chart_types/xy_chart/state/selectors/get_debug_state.ts
Outdated
Show resolved
Hide resolved
packages/charts/src/chart_types/xy_chart/state/selectors/get_debug_state.ts
Outdated
Show resolved
Hide resolved
: { | ||
id: annotation.id, | ||
color: annotation?.style, | ||
data: annotation.dataValues[index], |
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.
This is an invalid array access: the index
is the index of the annotationSpecs
array, but here you want to enumerate the dataValues
array. You should rewrite the code to return an DebugStateAnnotations
per each element into the dataValues
array of each spec
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.
Ok awesome I think I'm following commit isn't showing on github right now but it's 0d4024 in my local environment and it looks like the CI is running.
For the click handle rect story with multiple rectangle and a line annotation, the debug state looks as follows:
'annotations':
[{'data':{'coordinates':{'x0':0,'x1':1,'y0':0,'y1':4},'details':'details about this annotation'},
'id':'rect1',
'color':{'stroke':'#FFEEBC','strokeWidth':0,'opacity':0.25,'fill':'red'},
'type':'rectangle'},
{'data':{'coordinates':{'x0':0.8,'x1':2,'y0':1,'y1':5},'details':'details about this other annotation'},
'id':'rect2',
'color':{'stroke':'#FFEEBC','strokeWidth':0,'opacity':0.25,'fill':'blue'},
'type':'rectangle'},
{'data':{'dataValue':2,'details':'detail-0'},
'id':'line_annotation',
'color':{'line':{'stroke':'#777','strokeWidth':1,'opacity':1},
'details':{'fontSize':10,'fontFamily':'sans-serif','fontStyle':'normal','fill':'#777','padding':0}},'type':'line',
'domainType':'xDomain'}
]}">
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.
LGTM, please review the changes I added in c4dc3a9.
packages/charts/src/index.ts
Outdated
export { | ||
DebugState, | ||
DebugStateLine, | ||
DebugStateValue, | ||
DebugStateAnnotations, | ||
DebugStateArea, | ||
DebugStateAxes, | ||
DebugStateBar, | ||
DebugStateLegend, | ||
} from './state/types'; |
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 know that we really need to export all of these. Generally I look at it as "is someone gonna use this on its own". For instance the Theme
people may define LegendStyles
on its own and thus would require us to export it.
In this case however, I see the debug state being readonly, such that people say this state value should be x, and not setting downstream values of the debug state.
@markov00 thoughts?
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.
Isn't easier for them to know the shape of the debug state if we export the used types?
packages/charts/src/chart_types/xy_chart/state/selectors/get_debug_state.ts
Outdated
Show resolved
Hide resolved
jenkins test this please |
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.
It looks good for me!
Thanks for all the changes on the exposed APIs
# [40.0.0](v39.0.2...v40.0.0) (2021-11-18) ### Bug Fixes * **interactions:** remove the option for pixelRatio with png snapshot ([#1431](#1431)) ([eebb069](eebb069)) * **xy:** occlude points outside of y domain ([#1475](#1475)) ([3176f02](3176f02)) ### Features * **annotations:** add annotations to DebugState ([#1434](#1434)) ([c5ea600](c5ea600)) * **heatmap:** add valueShown in heatmap debug state ([#1460](#1460)) ([962e089](962e089)) ### BREAKING CHANGES * **interactions:** The getPNGSnapshot function no longer has an option for pixelRatio
Summary
Annotations are now added to debugState.
Details
The
DebugStateAnnotations
interface includes a data attribute which ultimately takes in an any type. I would like to restrict this if possible but it's from the dataValues off the LineAnnotationSpec or RectAnnotationSpec.Issues
Closes #1260
Checklist
:interactions
,:axis
)closes #123
,fixes #123
)packages/charts/src/index.ts