-
Notifications
You must be signed in to change notification settings - Fork 81
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
Added DocsExample #836
Added DocsExample #836
Conversation
Hi @MisRob, KCardGrid.-.Kolibri.Design.System.-.Google.Chrome.2024-11-21.00-13-45.mp4 |
Hi @chetan21122004, thanks a lot! We will review, likely during the next week. |
Hello @chetan21122004, even though I may chime in occasionally, my colleague-friend @ozer550 will be primarily reviewing and coordinating with you when he's back at work some time next week. Thank you @ozer550 :) |
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.
Hi @chetan21122004 this is good work! Just conveyed some of the changes after talking with the team. The changes include:
- Changes to Toggle button.
- Enchancements in the DocsExample component.
Feel free to ask any questions you have!
docs/common/DocsExample.vue
Outdated
|
||
<div class="code-toggle-button"> | ||
<button @click="toggleCodeVisibility"> | ||
<span></></span> |
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.
Instead of this, we could use KIconButton component. After discussing with the team mainly there are three things that we need to do here.
- We need to add the new icon for code toggle, instructions for which are written here. You can download the required svg file from here.
- Currently once we click on the toggle button the symbol of code does not change. It’s clear that the
</>
represents code but its not sure that pressing that symbol again would change it back to the visual example. So to improve this experience we would want to change the symbol of the code icon to chevronUp. Meanning we want to change the code icon to chevronUp when they click to see the code and then again revert back to code icon when they want to just see the visual example. - The
KIconButton
also has a tooltip prop. We want to add a tooltip for the toggle button for better user experience too. It should say something like 'show/hide code'.
docs/common/DocsExample.vue
Outdated
</div> | ||
|
||
<!-- Toggle code --> | ||
<div v-if="isCodeVisible" class="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.
Also If we could change the v-if
here to v-show
as elements shown with v-show are mounted once and remain in the DOM.
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.
Hi @ozer550,
Thanks for the detailed feedback! I’ve made the changes based on your suggestions. Here's a summary of what I’ve done:
- Replaced the toggle button with KIconButton: I've updated the button to use the new component.
- Updated the toggle icon: The icon now switches between < > and chevronUp when toggling the code visibility.
- Added tooltip: The tooltip for the toggle button now says "Show/Hide Code" for better user experience.
- Changed v-if to v-show: I've updated this to ensure the element remains in the DOM after being toggled.
newDocExample.mp4
Hi @ozer550, I’ve completed the task but mistakenly commented in the conversation instead of a new one. Please review! |
Hi @chetan21122004! Thanks for the changes will review this ASAP. |
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.
Hi @chetan21122004! We would need to rework on this PR as I see some unwanted changes from your last commit probably due to some misconfiguration or formatting tool that you are using locally.
- I see a lot of changes to files in
precompiled-icons/le/files
. This should not have happened. We would need to revert them. - Big diffs in files like
kcardgrid.vue
andiconDefinitions.js
. This is probably due to formatting tool that you are using or some misconfiguration with it.
We need to do the following things to fix it:
- Revert the changes and trying adding the icon again and see if the script still makes those unwanted changes.
- Resolve the linting check failure and run 'yarn lint-fix'.
Please feel free to ask any follow up questions necessary.
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.
This import seems to be broken. It should be KIconButton instead.
@@ -7,6 +7,6 @@ | |||
|
|||
<script> | |||
|
|||
export default {"name":"icon-40bcababd163c5d974599a8102319c63"} | |||
export default {"name":"icon-90cbb33ba1b625a32a6da324f4db40b2"} |
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.
We should not see changes to the files in precompiled-icons/le
folder. Idk why this happened. We would need to revert them back. The only change that should be present is lib/KIcon/precompiled-icons/le/codeToggle.vue
. Could you revert the commit and try adding the icon again and see if the script generates the same results?
docs/pages/kcardgrid.vue
Outdated
@@ -1,907 +1,950 @@ | |||
<template> |
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 huge diff which should not be present. This is probably due to some local formatting tool? Could you first resolve the linting check fails and then run yarn lint-fix
.
lib/KIcon/iconDefinitions.js
Outdated
@@ -1,579 +1,582 @@ | |||
import { themePalette, themeTokens } from '../styles/theme'; |
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.
Same case here too!
8bab16c
to
fad3c15
Compare
Hi @ozer550 , Apologies for the confusion! I accidentally deleted the branch and close the issue, but I've restored the branch. I’ve also addressed the requested changes , including reverting the unwanted changes and fixing the linting errors. Thanks for your support! |
Hi @ozer550 , I am facing few issues in DocsExample branch can i just make new branch and submit a PR |
Hi @chetan21122004, thanks for checking on it - yes feel free to open a new branch if it's easier. I'd ask you to reference this PR in the new PR's description so we can track the review. |
Also @chetan21122004, after you start the new branch from the latest |
No worries @chetan21122004! Looking at the new changes we are almost there! |
I will close this in favor of @chetan21122004's #873 where I responded the comments and posted further feedback. |
Description
I’ve added the DocsExample component that allows toggling between an example and its code.
Issue Addressed
Fixes #826.
Changelog
-Description: Added DocsExample component to toggle between example and code.
-Products Impact: -
-Addresses: #826.
-Components: DocsExample.
Guidance: The new component enhances the documentation UI by toggling code visibility.
Steps to Test
1.Navigate to the KCardGrid documentation page.
2.Verify that the example and code toggle correctly with a slide-up/slide-down animation.
Implementation Notes
-The DocsExample component uses a #code slot to accept code snippets in multiple languages.
-Slide toggle animation is applied when showing/hiding the code.