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

website: Update tooltip documentation & demos #1205

Merged
merged 20 commits into from
May 8, 2023

Conversation

idakukimiya
Copy link
Contributor

  • Update documentation
  • Remove placeholder

@idakukimiya idakukimiya requested a review from a team as a code owner April 12, 2023 14:45
@idakukimiya idakukimiya requested review from a team, FlyersPh9 and LostABike and removed request for a team April 12, 2023 14:45
@idakukimiya idakukimiya self-assigned this Apr 12, 2023
@idakukimiya idakukimiya added the documentation Improvements or additions to documentation label Apr 17, 2023
@gretanausedaite gretanausedaite requested review from gretanausedaite and removed request for FlyersPh9 April 18, 2023 10:46
Copy link
Contributor

@LostABike LostABike left a comment

Choose a reason for hiding this comment

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

I think this looks good now Oops lol

Copy link
Contributor

@LostABike LostABike left a comment

Choose a reason for hiding this comment

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

Sorry my bad. I think the format is supposed to be description/comments and then example below, not example and then description. I misread the descriptions so I didn't catch this before. Sorry :x

Comment on lines 38 to 42
<LiveExample src='Slider.tooltipnone.tsx'>
<AllExamples.SliderTooltipNoneExample client:load />
</LiveExample>

Tooltips are enabled by default but can be turned off.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant to group the demo for "tooltip in Slider" and "tooltips can be disabled" into one example here but that would be a little confusing. Maybe we can separate it? So normal slider with tooltip belong to the first group of examples. And then another slider without tooltip to demonstrate that tooltips can be disabled. I'm not too sure, maybe we can have a second opinion on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I think these stepper and slider examples don't even belong on this page. Just links to those specific examples in their own pages should be enough.

@LostABike LostABike self-requested a review May 1, 2023 17:42
apps/website/src/pages/docs/tooltip.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/tooltip.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

Couple more comments and should be good to go.

<AllExamples.TooltipPlacementExample client:load />
</LiveExample>

- `left`
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove placement list. It's a bit out of place and not fully correct.

apps/website/src/pages/docs/tooltip.mdx Outdated Show resolved Hide resolved
@idakukimiya idakukimiya added this pull request to the merge queue May 8, 2023
Merged via the queue into main with commit 043863d May 8, 2023
@idakukimiya idakukimiya deleted the ida-documentation-tooltip branch May 8, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants