-
Notifications
You must be signed in to change notification settings - Fork 92
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
DocsExample
: Add dynamic loading of components and display of code blocks
#918
base: develop
Are you sure you want to change the base?
Conversation
This PR introduces a bare-bones example of the now new component that can be easily used by refactoring the (I am just listing them here so that they are documented as they are fresh in my mind right now. I would be happy to address the relevant ones as per the feedback in this PR itself):
|
Update: I have tried to address most of the points I mentioned yesterday in this PR! You can check out how the new component fits into the documentation ecosystem by browsing the following documentation pages from the deploy preview:
You can let me know if any changes in the appearance/theme of the new component are needed! Also, as for the points left unchecked:
I feel that instead of introducing this functionality in the
This is more sort of a long-running change that we can open PSNow, the PR ends up actually making 3 net changes to KDS:
We should probably add three different changelog items for the same? Does the current automated workflow support that or should I do that part manually for this PR? |
Hi @EshaanAgg, thanks a lot! We took a peek by chance at the example pages you prepared during one of our meetings, and lots of excitement :) |
@marcellamaki and/or @nucleogenesis would you be available to review as a whole? I think you both should have enough context from our meetings, but in case it's useful to sum it up - this work builds on top of #826 and resolves several issues actually #845 #861. Conversations that lead to this final interface are in many places. Based on the feedback I collected few weeks ago among team members and previous chats with @EshaanAgg, we agreed on what he proposed here and from my perspective, this PR is very well aligned with it. |
Thanks all |
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 left a question about the Github link. I think that our existing docs component hard-codes the Github SVG into the component. If we could use that, then you could avoid adding the icon to precompiled-icons/le and iconDefinitions.
Overall this is really fantastic and makes the docs page look & feel very clean. I love the approach to splitting things up and will make it easy to make updates and additions moving forward! Thanks @EshaanAgg
docs/common/DocsExample.vue
Outdated
<KIconButton | ||
v-if="loadExample" | ||
appearance="raised-button" | ||
icon="github" | ||
tooltip="View the complete code example on GitHub" | ||
@click="redirectToGitHub" | ||
/> |
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 DocsGithubLink work here?
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.
The same creates an anchor text link to a GitHub page, which is only for issues and pull requests. Here, we want to create a KIconButton
, which is not possible through that component directly.
Though seeing that the GitHub SVG is inlined in the same, I can make either of the two changes:
- Change the
DocsGithubLink
to use this pre-compiled SVG that I have added. - Remove the
github
icon from precompiled icons, and inline the same inDocsExample
.
I personally would prefer the first option over the second, but I would be happy to implement the one that feels more appropriate to the team!
Made another minor improvement: Now the component makes use of the
|
Hi @EshaanAgg - thanks for your patience while I get to review. I'll add comments by tomorrow - this looks great and we appreciate the contribution! |
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 @EshaanAgg - this is looking really good. Essentially, I don't really see any major issues, but since this is a documentation update, I've added a few questions/comments, and made note of a few considerations I also want to ask other reviewers (but I welcome your suggestions/perspective as well). I'll re-review next week after we've had some time for other thoughts. Thanks very much for your contributions.
/* | ||
* Unique identifier for the example. This must be unique within | ||
* the scope of the documentation page across all the DocsExample components. | ||
* @type {String} |
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 not required, but I wonder if it would be useful (first as the KDS maintainers, and then in this comment) to provide some guidance about what this id/string should look like, especially as we want to be unique within all docs examples. Presumably, the implementer would be creating an exampleId for the purposes of using this component right? It's not a database UUID. If it's too simple, i.e. the first example id is 1
and we just keep incrementing, that might be hard to track over time.
So.... Should we use the name of the component (as those are unique) and if there are variations, append a -
and then an integer? Should we suggest randomly generating a UUID? These are really more of questions for the team - I'll raise them. But if you have any thoughts (or clarifying points - it's possible I don't fully understand the role of the example ID here) - that would be very welcome :)
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.
My bad here! The documentation comment is not very well written here and conveys the wrong intent: the exampleId
does not need to be unique across all the usages of the DocsExample
across all the pages, but only on the page on which the component is rendered.
Internally, the same is passed as tabId
to the KTabs
component, which has the following guidance associated with it:
An ID of a tabbed interface represented by this component. Needs to be be unique in regards to all tabbed interfaces rendered on one page.
I will update the documentation comment to convey the same intent and add an example!
if (this.$slots.javascript || scriptContent) { | ||
tabList.push({ | ||
id: 'js-codeblock', | ||
label: 'Script', |
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'm a little bit on the fence about this in terms of personal preference, so I am going to see if any other reviewers have strong feelings, but my leaning is to have this label be 'Javascript'. I know that in the vue templates, it's a <script> tag, and in some ways having that match makes the most sense. At the same time.... it's javascript. So, it just feels a little more clear. But, I welcome alternate opinions, from you or others on the team.
/* | ||
* Returns the tooltip message for the GitHub icon | ||
*/ | ||
githubTooltip() { |
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 wonder if rather than adding the caveats (not available in PRs/not available locally), we just don't display this in those environments? So, only display the icon in prod? I guess the downside of that would be confirming that it's working in develop....
Description
This PR introduces the dynamic loading of example components via the
DocsExample
component.Issue addressed
Fixes part of #845
Changelog
DocsExample
DocsExample
visual output and API #845Implementation notes
I have ensured that the component follows the API specification described in this comment.
Does this introduce any tech-debt items?
No
Testing checklist
Reviewer guidance