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

DocsExample: Add dynamic loading of components and display of code blocks #918

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

Conversation

EshaanAgg
Copy link
Contributor

Description

This PR introduces the dynamic loading of example components via the DocsExample component.

Issue addressed

Fixes part of #845

Changelog

Implementation 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

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

@EshaanAgg
Copy link
Contributor Author

EshaanAgg commented Jan 31, 2025

This PR introduces a bare-bones example of the now new component that can be easily used by refactoring the KTable component and an example in the KCardGrid documentation to show its versatility. If the same seems to be satisfactory, there can be many more improvements that we can add to the same:

(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):

  • Improve the styling of the component to match the documentation theme a bit more
  • Add animations when sliding toggling code visibility
  • Add animations when switching between different code blocks
  • Disable ESLint rules like no-console-log for the examples directory, and then remove the eslint-disable from the KTable example
  • Update the developer documentation about the API of DocsExample and how we can use it to build better documentation pages
  • Add a button to link to the complete GitHub code example file alongside the code toggle button
  • Migrate more documentation to make use of DocsExample and collect feedback to see if it is a satisfactory solution to avoid the problems listed in Configure prettier and eslint to treat DocsShowCode as code examples #861 to an appreciable extent!

@EshaanAgg
Copy link
Contributor Author

EshaanAgg commented Feb 1, 2025

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:

Add animations when switching between different code blocks

I feel that instead of introducing this functionality in the DocsExample component directly, it will be better added as a feature to the KTabs component itself. We can allow the user to configure a props like animate: true, or other custom options like animationDuration and animationEaseMode, and then use that behaviour here! If this seems like a viable plan, I can open a new issue for the same.

Migrate more documentation to make use of DocsExample and collect feedback to see if it is a satisfactory solution to avoid the problems listed in #861 to an appreciable extent!

This is more sort of a long-running change that we can open good-first issues for the it when the PR is stably merged!

PS

Now, the PR ends up actually making 3 net changes to KDS:

  • Adding a new icon (github) to KDS
  • Adding a new type of transition to KTransition
  • Updating the API of DocsExample

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?

@nucleogenesis nucleogenesis requested review from MisRob and removed request for MisRob February 4, 2025 17:36
@MisRob
Copy link
Member

MisRob commented Feb 5, 2025

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 :)

@MisRob
Copy link
Member

MisRob commented Feb 5, 2025

@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.

@MisRob
Copy link
Member

MisRob commented Feb 5, 2025

@ozer550 since you're familiar with the predecessor #826, you're very welcome to join the review fully or just some parts of it :)

@MisRob MisRob added the TODO: needs review Waiting for review label Feb 5, 2025
@MisRob
Copy link
Member

MisRob commented Feb 5, 2025

Thanks all

Copy link
Member

@nucleogenesis nucleogenesis left a 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

Comment on lines 8 to 14
<KIconButton
v-if="loadExample"
appearance="raised-button"
icon="github"
tooltip="View the complete code example on GitHub"
@click="redirectToGitHub"
/>
Copy link
Member

Choose a reason for hiding this comment

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

Could DocsGithubLink work here?

Copy link
Contributor Author

@EshaanAgg EshaanAgg Feb 7, 2025

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 in DocsExample.

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!

@EshaanAgg
Copy link
Contributor Author

EshaanAgg commented Feb 10, 2025

Made another minor improvement: Now the component makes use of the environment.js to get the deploy context and handles the GitHub button click appropriately:

  • In local and PR environments, clicking on the button does not redirect the user anymore (to potentially broken links), and the tooltip on the button informs the user about the same.
  • When targeting a branch on KDS, the same use the environment.url variable to populate the path to the example. This would be handy (in the off chance) if someone wants to deploy their own fork to Netlify

@marcellamaki
Copy link
Member

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!

Copy link
Member

@marcellamaki marcellamaki left a 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}
Copy link
Member

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 :)

Copy link
Contributor Author

@EshaanAgg EshaanAgg Feb 14, 2025

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',
Copy link
Member

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() {
Copy link
Member

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....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants