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 miscellaneous content issues with MDX docs #1304

Merged
merged 8 commits into from Oct 7, 2021

Conversation

jamesfan
Copy link
Member

@jamesfan jamesfan commented Oct 7, 2021

Summary

Fixes #1305.

category


Checklist

  • Sync Pagination and Tabs MDXes to use consistent language
  • Fix Popup MDX structure
  • Standardize headings (RTL examples, Props tables)
  • Update Radio Button examples to follow case guidance
  • Standardize references to HTML elements (<button> instead of button)

@jamesfan jamesfan added the documentation Effects documentation label Oct 7, 2021
@jamesfan jamesfan self-assigned this Oct 7, 2021
@jamesfan jamesfan added this to In Progress in Current Sprint (7/20 - 8/9) via automation Oct 7, 2021
@jamesfan jamesfan removed this from In Progress in Current Sprint (7/20 - 8/9) Oct 7, 2021
@cypress
Copy link

cypress bot commented Oct 7, 2021



Test summary

624 0 1 0


Run details

Project canvas-kit
Status Passed
Commit 144f0b1 ℹ️
Started Oct 7, 2021 3:39 AM
Ended Oct 7, 2021 3:45 AM
Duration 06:01 💡
OS Linux Ubuntu - 20.04
Browser Electron 85

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@@ -23,8 +23,10 @@ import {
# Canvas Kit Pagination

`Pagination` is a
[compound component](/getting-started/for-developers/resources/compound-components/) for handling
navigation between pages in a range.
[compound component](/getting-started/for-developers/resources/compound-components/) that allows
Copy link
Contributor

Choose a reason for hiding this comment

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

Subtle touch up here, I like it.

@@ -20,8 +20,8 @@ import {
# Canvas Kit Tabs

`Tabs` is a [compound component](/getting-started/for-developers/resources/compound-components/)
which can be composed in a variety of different ways. It follows the
[W3 Tabs specification](https://www.w3.org/TR/wai-aria-practices/#tabpanel).
that allows users to navigate between related views of content while remaining in context of the
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

// Configuration
<Tabs onActivate={({data}) => console.log('Activated tab', data.tab)}>{/* Subcomponents */}</Tabs>;
<Tabs onActivate={({data}) => console.log('Activated tab', data.tab)}>
{/* Child components */}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking on this along with all the other JSX changes that turned /* Subcomponents */ into /* Child components */ and I think I prefer these commented as "subcomponents." As a React developer I understand them to be "child components" based on their placement in the JSX. I feel like indicating that this is specifically where the subcomponents go connects the dots very directly, but if it says child components, there's a small but necessary mental connection to be made, that might not be completely obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point @willklein. I changed the language from /* Subcomponents */ to /* Child components */ to be more technically correct since it's possible for children to not be Subcomponents (for example, the wrapper <div> around the tab panels in the Basic Tabs Example). I wonder if it would make sense to sacrifice a bit of correctness here in exchange for a more educational example.

Or maybe there's language which is both correct and does a good job of tying container components to their subcomponents? /* Subcomponents (and other child components) */ or /* Child components (including subcomponents) */?

Copy link
Contributor

@willklein willklein left a comment

Choose a reason for hiding this comment

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

There was one thing ("child components" -> "subcomponents" in JSX) that I didn't like, but everything else is great and this is approved as is.

@workday-canvas-kit workday-canvas-kit merged commit a682663 into Workday:master Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge documentation Effects documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix miscellaneous content issues with MDX docs
3 participants