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

Migrate KTabList examples to DocsExample #956

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

Conversation

mukulpythondev
Copy link

@mukulpythondev mukulpythondev commented Mar 10, 2025

Description

This PR migrates the KTabsList documentation to use the DocsExample component for better structure and maintainability. The change ensures that examples are properly organized while keeping the documentation concise and functional.

Issue addressed

Addresses #953

Before/after screenshots

Before:
image
image
image
image

After:
image
image

Changelog

Steps to test

  1. Open the KTabsList documentation page.
  2. Verify that all examples load correctly using DocsExample.
  3. Ensure the collapsible/toggle functionality works as expected.
  4. Check that no examples are missing and the formatting remains intact.

(Optional) Implementation notes

At a high level, how did you implement this?

  • Moved example code to docs/examples/KTabsList/ directory.
  • Replaced inline examples with the DocsExample component.
  • Ensured documentation structure remained unchanged.

Does this introduce any tech-debt items?

No.

Testing checklist

  • Contributor has fully tested the PR manually.
  • If there are any front-end changes, before/after screenshots are included.
  • Documentation content remains functional and correctly formatted.

Reviewer guidance

  • Is the documentation correctly structured with DocsExample?
  • Are all previous examples accounted for and functional?
  • Is the example styling consistent with other components?

Comments

(Any additional notes for the reviewer.)

@MisRob
Copy link
Member

MisRob commented Mar 13, 2025

Welcome and thanks @mukulpythondev, we will review.

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.

Thanks @mukulpythondev for the contribution. I have added some comments as a preliminary review. Also it seems like all the files changed in this PR have not been properly linted. You can use the command yarn run lint-fix after making the necessary changes to automatically fix the formatting in all of the files. This would also help the CI/CD to pass before the final review.

Thanks for your contribution.

Comment on lines 2 to 9
<div
:style="{
backgroundColor: $themePalette.grey.v_700,
padding: '10px 20px',
color: $themeTokens.textInverted,
display:'inline-block'
}"
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding this custom styling to this particular example? As far as I understand, we want to show the styling capabilities of the props in this example, and thus adding styles to the parent div kind of distracts the focus from the same.

Can we remove this parent div completely and just keep the KTabsList component in the template?

@@ -0,0 +1,35 @@
<template>
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the wrapping div since it's empty anyhow (has no styling or functionality associated with it)?

@@ -0,0 +1,38 @@
<template>
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about wrapping empty div.

@@ -0,0 +1,29 @@
<template>
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Comment on lines 274 to 292
export default {
name: 'DocsKTabsList',
data() {
return {
tabs: [
{ id: 'tabLessons', label: 'Lessons' },
{ id: 'tabLearners', label: 'Learners' },
{ id: 'tabGroups', label: 'Groups' },
],
ex1activeTabId: 'tabLessons',
icons: {
tabLessons: 'lesson',
tabLearners: 'person',
tabGroups: 'people',
},
};
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove all these exported data members (as all of the examples and the code needed to run them has been migrated to individual files)?

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.

Thanks for all the changes @mukulpythondev. They all seem on the right track and very close to as we would finally like them. I have left some minor comments, and would love it it you can address them.

Also the changes right now do not seem to be linted. Please do run the linting script to autofix all of them for you!

Feel free to request a review after making all the changes, or ask for clarifications if you need any. Thanks again for all your work.


export default {
data() {
return sharedExampleData; // Use shared data
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice thinking here @mukulpythondev trying to minimise the code duplication across examples.

I would welcome this change if it were in some other part of the codebase, but here I feel the verbosity of having the data copied in each example file it worth it. There are actually a couple of reasons for it:

  • The whole point of these documentation examples is to be standalone and capable of independent rendering. If we reference constants from other files, the same defeats the spirit of the same.
  • If we add the tab data to each example, then the documentation viewers would actually see that data, it's structure and it's direct correspondance with the component when they view the script part of the example. However if we make use of an import like this, the actual structure of the imported data may never visible to them, which would kind of defeat the purpose of this documentation.

Thus I would favour the copying and pasting of the shared data in each of the examples seperately.



<script>
import { sharedExampleData } from '../../pages/ktabslist.vue';
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto



<script>
import { sharedExampleData } from '../../pages/ktabslist.vue';
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto



<script>
import { sharedExampleData } from '../../pages/ktabslist.vue';
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

:activeTabId="ex1activeTabId"
/>
</DocsShow>
<DocsExample
Copy link
Contributor

Choose a reason for hiding this comment

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

We recently merged #965 into the codebase which make changes to the way the DocsExample component renders. You may need to add the 'block' attribute to the same post the migration to preserve the look of the page.

Please do pull the latest changes from the KDS repository and then make the next set of changes to avoid any merge conflicts.

@mukulpythondev mukulpythondev force-pushed the migrate-ktabslist-to-docsexample branch from d094d28 to 0461531 Compare March 21, 2025 03:33
@MisRob MisRob requested a review from EshaanAgg April 1, 2025 14:24
@MisRob
Copy link
Member

MisRob commented Apr 1, 2025

Hi @mukulpythondev, just checking in, is all feedback resolved or would you need more time?

@mukulpythondev
Copy link
Author

mukulpythondev commented Apr 1, 2025

@MisRob All fix and working fine only 1 UI difference one background color because first i do with directly with the css but it is not aligning with our previous code and i can not find any method to do that.
Rate my Approach to fix this give one more prop for the docsexample component.
if any help from mentor (@EshaanAgg ) or contributors.

Current on Website :
image

My PR :
image

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