Skip to content

Conversation

@pan-kot
Copy link
Member

@pan-kot pan-kot commented Aug 13, 2025

Description

Depends on cloudscape-design/documenter#85

How has this been tested?

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@pan-kot pan-kot marked this pull request as ready for review August 14, 2025 08:46
@pan-kot pan-kot requested a review from jperals as a code owner August 14, 2025 08:46
"analyticsTag": undefined,
"defaultValue": undefined,
"deprecatedTag": undefined,
"description": "Defines the minimum allowed width of the chart plot. When the parent container is smaller the
Copy link
Member

Choose a reason for hiding this comment

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

Can be fixed separately, but there is a little typo in this description: the word "than" is missing between "smaller" and "the". In the previous description it is correct.

Besides, to be even more precise, instead of "smaller" we could say "narrower" or "shorter" depending on the case.

"analyticsTag": undefined,
"defaultValue": undefined,
"deprecatedTag": undefined,
"description": "Use Cloudscape keyboard navigation, \`true\` by default.",
Copy link
Member

Choose a reason for hiding this comment

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

Could defaultValue be true instead of having it indicated in the description?

"defaultValue": undefined,
"deprecatedTag": undefined,
"description": "Controls the placement of the vertical axis title.
When set to "side", displays the title along the axis line.",
Copy link
Member

Choose a reason for hiding this comment

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

There are unescaped double quotes here inside a string delimited in double quotes. Will this work as expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it should. I think this is just the format the snapshot uses. We use double quotes in existing api docs, no problem with that.

@pan-kot pan-kot requested a review from jperals August 26, 2025 09:05
export { CoreChartProps };

function CoreChart(props: CoreChartProps) {
function CoreChart({ keyboardNavigation = true, ...props }: CoreChartProps) {

Choose a reason for hiding this comment

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

Why is the keyboardNavigation handled separately from the rest of the props in the function?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to assign the default value. When done in the index file - this can be parsed by the documenter. You can check the documenter snapshot - now the default value for keyboardNavigation is defined.

@pan-kot pan-kot enabled auto-merge August 29, 2025 06:55
@pan-kot pan-kot requested a review from teodoranemes August 29, 2025 06:55
@pan-kot pan-kot added this pull request to the merge queue Aug 29, 2025
Merged via the queue into main with commit 924cbf8 Aug 29, 2025
36 of 38 checks passed
@pan-kot pan-kot deleted the fix-core-documenter-snapshot branch August 29, 2025 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants