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.

Sign up for GitHub

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

Jump to bottom

Migrate KImg documentation to use DocsExample component #962

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Migrate KImg documentation to use DocsExample component #962

wants to merge 3 commits into from

Conversation

Pandaa007
Copy link
Contributor

Description

  • Fixed the bug with the use of scss language in the DocsShowCode component
  • Migrated the examples on the KImg page to use DocsExample wherever possible

Issue addressed

Fixes #952

Changelog

Implementation notes

While migrating the examples, I was unable to migrate two of the examples listed in the KImg documentation page as they relied on being rendered in an inline element like span, which by default the DocsExample component renders the same in a block div element. This led to different looking outputs for those particular examples.

It would be nice if the DocsExample component also support a block attribute much like DocsShow, so the end consumer can control out the output is to be shown to the user. I can make the required changes in this PR itself, or open a new PR to address the same.

cc: @MisRob @EshaanAgg

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

EshaanAgg
EshaanAgg approved these changes Mar 15, 2025
View reviewed changes
Copy link
Contributor

@EshaanAgg EshaanAgg left a comment

Choose a reason for hiding this comment

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

The changes look good to me @Pandaa007! The documentation page also looks much better than it previously did. Thanks for your work here.

I will tag @MisRob for additional review, and to once get a second opinion on the before and after appearance of the documentation page. Post that, we can merge this.

@EshaanAgg EshaanAgg requested a review from MisRob March 15, 2025 07:02
@EshaanAgg
Copy link
Contributor

Implementation notes

While migrating the examples, I was unable to migrate two of the examples listed in the KImg documentation page as they relied on being rendered in an inline element like span, which by default the DocsExample component renders the same in a block div element. This led to different looking outputs for those particular examples.

It would be nice if the DocsExample component also support a block attribute much like DocsShow, so the end consumer can control out the output is to be shown to the user. I can make the required changes in this PR itself, or open a new PR to address the same.

cc: @MisRob @EshaanAgg

Thanks for pointing out this limitation of the DocsExample component! I think it would be better to open a new issue for the same, and make those changes in a new PR to keep the scope of this PR limited and easier to review. We can migrate the leftover examples from KImg in the new PR addressing the block attribute issue as a sanity check for the changes made. The best way to address the same might be to ensure that we internally make use of DocsShow to wrap the rendering example, and "forward" the block prop from the DocsExample component to the DocsShow component.

What do you think about the same @MisRob?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EshaanAgg EshaanAgg EshaanAgg approved these changes

@MisRob MisRob Awaiting requested review from MisRob

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate KImg documentation to use DocsExample component
2 participants
@Pandaa007 @EshaanAgg