-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add DocsExample #873
base: develop
Are you sure you want to change the base?
Add DocsExample #873
Conversation
CHANGELOG.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chetan21122004 Would you remove any updates of CHANGELOG.md
from this pull request? It will get automatically updated after we merge.
It's best to not remove the changelog section from the pull request description and fill it in there. That's the source that will be used for the changelog updated. I've just prepared it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice way to reset a file git checkout develop -- CHANGELOG.md
https://stackoverflow.com/questions/38743912/remove-a-file-from-a-git-pull-request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chetan21122004 and @ozer550, wonderful work so far. Posting few in-code comments, mostly last touches. Then I think we could be ready for merge.
docs/common/DocsExample.vue
Outdated
padding-top: 0; | ||
padding: 20px; | ||
|
||
/* border: 1px solid #ddd; */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed
docs/common/DocsExample.vue
Outdated
padding: 20px; | ||
|
||
/* border: 1px solid #ddd; */ | ||
border-radius: 4px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're not showing the border, can be removed too
docs/common/DocsExample.vue
Outdated
justify-content: flex-end; | ||
} | ||
|
||
.code-toggle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest all these styles are removed in favor of using KIconButton
as is. For consistency and simpler styles.
docs/common/DocsExample.vue
Outdated
|
||
|
||
|
||
.code-container { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This style doesn't take any effect - can be removed
docs/common/DocsExample.vue
Outdated
|
||
|
||
<div class="code-toggle-button"> | ||
<KIconButton class="code-toggle" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use <KIconButton appearance='raised-button' ...>
Then you can remove code-toggle
class. Note this will also change the button background color to light gray, which is desired. The yellow is a bit too much. Code toggle is not as important element on the documentation page overall. You can see KIconButton docs.
</div> | ||
|
||
<!-- The component --> | ||
<div class="example-container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example-container
is an unused class, can be removed
docs/common/DocsExample.vue
Outdated
|
||
|
||
<script> | ||
import KIconButton from "../../lib/buttons-and-links/KIconButton.vue"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there's no need to import KIconButton
for it to work since it's globally registered. Could you try remove please and see if it still works?
docs/pages/kcardgrid.vue
Outdated
@@ -964,10 +765,12 @@ | |||
|
|||
import useKResponsiveWindow from '../../lib/composables/useKResponsiveWindow'; | |||
import DocsKCard from '../pages-components/DocsKCard'; | |||
import DocsExample from '../common/DocsExample'; // Corrected the import typo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you remove this import and instead register DocsExample
in the load-common-components.js
file? This will make DocsExample
available in all documentation pages without the need to import it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chetan21122004 let me push some changes here to help with this large diff - there are unrelated updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, done! It was simpler than explaining, there were a number of issues, some of them perhaps caused by our recent Node upgrade.
@chetan21122004 please see my latest commits and make sure to pull the branch to your local branch before you start resolving feedback. Would you double-check you don't have any local auto-formatting tools turned on before continuing work? Also before pushing, I'd ask you to always run yarn lint-fix
and preview that you're pushing changes only directly relevant to updates you work on - that should prevent this from happening again. Last thing, I only preserved the first DocsExample
usage as there were some issues and it was hard to review all places. Let's just keep this one place for the purpose of testing. We will introduce DocsExample
more widely in the scope of upcoming issues.
Hi @MisRob, Thank you for your detailed feedback. As a beginner, I found this task challenging and apologize for any inconvenience caused, including the unwanted changes I made as I didn’t use a formatter. I truly appreciate your patience and guidance. I will review your commits and make sure my setup avoids unrelated updates. I’m working on improving my coding practices and will run yarn lint-fix to ensure clean contributions. Your support means a lot, and I look forward to learning more from you. Thank you again! |
No problem @chetan21122004, I think lots of it was actually caused by the Node version transition. Let's see how it goes, now we have a clean state. Let us know if something unexpected would happen with linting. |
Hi @chetan21122004! Regarding the change log, yes it should not be present in the PR. Rather we need to fill in the changelog in the pull request template. For example here. |
Description:
I’ve added the DocsExample component that allows toggling between an example and its code.
Issue Addressed:
Fixes #826. Also references #836 for additional context on previous discussions related to the component.
Guidance:
The new component enhances the documentation UI by toggling code visibility.
Steps to Test:
Implementation Notes:
Changelog
DocsExample
component with toggle functionality for switching between an example and its code snippet in the KDS documentation.DocsExample
that allows to toggle between an example and its code #826