-
Notifications
You must be signed in to change notification settings - Fork 4
chore: Fixes output for chart-components core chart API #85
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
=======================================
Coverage 92.21% 92.22%
=======================================
Files 15 15
Lines 925 926 +1
Branches 273 274 +1
=======================================
+ Hits 853 854 +1
Misses 72 72 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/components/object-definition.ts
Outdated
| realType.flags & ts.TypeFlags.Number || | ||
| isArrayType(realType) || | ||
| realTypeName === 'HTMLElement' || | ||
| realTypeName.startsWith('SVG') || |
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.
The SVG elements are complex fail with the following error:
Error: Multiple declarations found for symbol: addEventListener
Having multiple function declarations is valid TS, but unsupported by documenter. There is also no reason to deep-expand native 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.
Still recommend full name matches, just in case if we have any our own type "SVGIcon" or something
src/components/object-definition.ts
Outdated
| isArrayType(realType) || | ||
| realTypeName === 'HTMLElement' || | ||
| realTypeName.startsWith('SVG') || | ||
| realTypeName.includes('Highcharts') || |
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.
The Highcharts objects are complex and recursive - those fails with Max call stack exceeded error.
There is also no good reason to expand these as the customers can use Highcharts docs for these instead. Also, atm. both SVG and Highcharts objects are only present in the unofficial chart-core component, not shown in the website. We generate docs for snapshots only.
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.
Similar comment here. Instead of includes it should be a full text. Or at least realTypeName.split('.')[0] === 'Highcharts'
just-boris
left a comment
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.
And there should be a unit test on this, of course
src/components/object-definition.ts
Outdated
| realType.flags & ts.TypeFlags.Number || | ||
| isArrayType(realType) || | ||
| realTypeName === 'HTMLElement' || | ||
| realTypeName.startsWith('SVG') || |
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.
Still recommend full name matches, just in case if we have any our own type "SVGIcon" or something
src/components/object-definition.ts
Outdated
| isArrayType(realType) || | ||
| realTypeName === 'HTMLElement' || | ||
| realTypeName.startsWith('SVG') || | ||
| realTypeName.includes('Highcharts') || |
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.
Similar comment here. Instead of includes it should be a full text. Or at least realTypeName.split('.')[0] === 'Highcharts'
@just-boris could you recommend a test design for this one? I attempted a snapshot test, for which I created a fixture like this: However, it generated an output that listed the SVGSVGElement type as is, w/o any changes to the code. Edit: I managed to make the snapshot test work with fake Highcharts.Point interface. |
8043b01 to
b2e38c9
Compare
|
I made the following updates:
|
Ok, good for now, approving |
src/components/object-definition.ts
Outdated
| realType.flags & ts.TypeFlags.Number || | ||
| isArrayType(realType) || | ||
| realTypeName === 'HTMLElement' || | ||
| realTypeName === 'Highcharts.Point' || |
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.
To make your life simpler, I still recommend checking realTypeName.split('.')[0] only to avoid editing this check any time someone uses a new type from Highcharts
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.
Makes total sense, updated!
Without these changes, the documenter fails in chart-components - see: cloudscape-design/chart-components#88
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.