-
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
DocsExample
: Add dynamic loading of components and display of code blocks
#918
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.
/* | ||
* 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....
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.
Thank you @EshaanAgg for your work here! This is a really fantastic piece of work, and I am sure that the code block display in particular will be really helpful both for our core team as well as external contributors from the open source community. We appreciate your contributions.
Wonderful news! @EshaanAgg on another issue I noticed you had interest with helping to organize some work and I also see you in reviews from time to time. Refactoring KDS documentation to use this component, as you noted, would be a good source good first issues for the community. If you'd be interested in taking on responsibilities such as overseeing this refactor as a whole project (e.g. opening issues, reviewing them, suggesting more improvements that other developers can implement), I'm more than happy to chat. No pressure at all, you have volunteered so much for us already :) I thought this could be perhaps interesting new area to explore. Just message me on Slack any time if this would sound interesting. |
Thanks, @marcellamaki @MisRob, for your guidance along the way! And, thanks @MisRob for the kind offer! I just love hanging out with the community members here, and helping in whatever small ways I can whenever I find the time. Helping to coordinate the overall documentation update is something that I would love. Will ping you soon about the same on Slack. As a quick post-merge consideration, I would like to just redraw your attention to these two points that I mentioned earlier in the conversation: One about opening a new issue for sliding animations:
Other about the changelog:
Please let me know if any changes regarding the same are needed in the context of this PR. |
Wonderful, thank you @EshaanAgg. Oh the changelog! I'm sorry for late reply. Yes it'd be best to have those things in the way you suggest @EshaanAgg. @AlexVelezLl uses this to create release notes - Alex, how would you recommend we proceed about the items that are missing since we merged before putting this together? |
@EshaanAgg I agree it'd be better to have |
Hey @MisRob! For release notes, it will be great to have these changelog items updated in the PR description even if the PR was already merged :), and if there is something super important to have in our Changelog file, I think opening another PR introducing these items directly to the changelog also do the work. For this specific case I have introduced these missing changelog items directly to the changelog file in the bump version PR, so no action needed for this! |
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