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

updated carousel docs. #1675

Merged
merged 39 commits into from
Nov 22, 2023
Merged

updated carousel docs. #1675

merged 39 commits into from
Nov 22, 2023

Conversation

siddhantrawal
Copy link
Contributor

Changes

Updated documentation for the Carousel component.

Testing

Docs

@siddhantrawal siddhantrawal requested review from a team as code owners November 6, 2023 21:18
@siddhantrawal siddhantrawal requested review from mayank99 and r100-stack and removed request for a team November 6, 2023 21:18
@siddhantrawal
Copy link
Contributor Author

Me and @rohan-kadkol think that this page looks complete. @mayank99 Do you think it is missing something? Not sure whether I should change subcomponents to composition API and define their props instead of them showing in a table?

apps/website/src/pages/docs/carousel.mdx Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
@siddhantrawal siddhantrawal self-assigned this Nov 6, 2023
@siddhantrawal siddhantrawal changed the title updating carousel docs. updated carousel docs. Nov 7, 2023
apps/website/src/pages/docs/carousel.mdx Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
Copy link
Member

@r100-stack r100-stack left a comment

Choose a reason for hiding this comment

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

Carousel.Navigation has a JSDoc comment saying the Carousel.Navigation must be placed before the slides (Carousel.Slider or custom slides) in DOM (#1626, #1626 (comment)). So the "Navigation" should likely mention that.

image

Another mention in Carousel's JSDocs:

image

UPDATE: Just realized that we have such comments in DotsList's JSDocs too. So probably need to mention this under the "DotsList" subheading too

image

apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
@siddhantrawal
Copy link
Contributor Author

siddhantrawal commented Nov 8, 2023

Carousel.Navigation has a JSDoc comment saying the Carousel.Navigation must be placed before the slides (Carousel.Slider or custom slides) in DOM (#1626, #1626 (comment)). So the "Navigation" should likely mention that.

image

Another mention in Carousel's JSDocs:

image

UPDATE: Just realized that we have such comments in DotsList's JSDocs too. So probably need to mention this under the "DotsList" subheading too

image

Added.

@@ -61,13 +61,13 @@ This component uses a composition approach, similar to most of the other compone

### Navigation

The `Carousel.Navigation` component by default consists of the `PreviousButton` and `NextButton` shown on the left and right, and the `Carousel.DotsList` component shown in the middle. `children` can be specified to override what is shown in this navigation section.
The `Carousel.Navigation` component by default consists of the `PreviousButton` and `NextButton` shown on the left and right and the `Carousel.DotsList` component shown in the middle. The navigation section must be present before the slides in DOM order.`children` can be specified to override what is shown in this navigation section.
Copy link
Member

Choose a reason for hiding this comment

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

Can the "must" part somehow be given more emphasis? Maybe we create an alert type component for this important piece of information? E.g. from the Astro Docs (link):

image

To do this, maybe we can re-purpose the <Placeholder>?

image

But for this PR at least, in the interest of time, you can just make the "must" portion emphasized some way. Maybe bold text? The Alert type can come later. Maybe we can make a mention of this idea in #961 to remember for the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

New paragraph starting with a bold "Important:"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

Copy link
Member

@r100-stack r100-stack left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/carousel.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

LGTM

@siddhantrawal siddhantrawal merged commit 718e5d2 into main Nov 22, 2023
14 checks passed
@siddhantrawal siddhantrawal deleted the siddhant/update_carousel_docs branch November 22, 2023 14:09
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