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

docs: allow DocsShow to automatically parse SFC's and display their source and output #905

Closed
wants to merge 7 commits into from

Conversation

EshaanAgg
Copy link
Contributor

@EshaanAgg EshaanAgg commented Jan 23, 2025

Description

The PR modifies the DocsShow component to accept two new arguments:

  • loadExample: The path to the component that needs to be shown to the user. When passed, the component is loaded dynamically, and its source code is parsed into three separate DocsShowCode blocks.
  • showOutput: Controls whether the example component is to be rendered in the documentation. Defaults to true.

All the examples must be in the /docs/examples directory to be loaded correctly. DocsShow still accepts a default slot, so the user can use it to render a custom component with or without the example. This functionality should (probably) be migrated to the DocsExample component when ready. The loadExample prop is set to be null by default so that it does not cause any regressions or changes in it's existing usage across the documentation pages.

I have also migrated the KTable documentation to use this new feature (as that is the component that I am most familiar with & believe could really benefit from this new API).

Issue addressed

Fixes #861

Changelog

Implementation notes

At a high level, how did you implement this?

Used the raw-loader plugin with Webpack to configure the loading of components.

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

@MisRob MisRob self-requested a review January 23, 2025 16:10
@MisRob MisRob self-assigned this Jan 27, 2025
@MisRob MisRob added the TODO: needs review Waiting for review label Jan 27, 2025
@MisRob
Copy link
Member

MisRob commented Jan 28, 2025

Thanks a lot @EshaanAgg, I will review. Would you please clarify if and how we can override the code output, for example how to achieve code sample outputs like here

Screenshot from 2025-01-28 19-54-05

It's something we do intentionally (and for larger components frequently) to prevent from overwhelming information. In the future, there will be a way to see the full via GitHub link, but documentation code samples are rather brief and to the point. During the course of this migration, I'd like to be sure we don't lose the flexibility in this area.

@MisRob
Copy link
Member

MisRob commented Jan 28, 2025

This functionality should (probably) be migrated to the DocsExample component when ready

Yes thank you, definitely. Basically DocsExample's final interface is planned to look like

<DocsExample>

  // The default slot renders the live example
  <KCardGrid ...>
    ...
  </KCardGrid>

  <template #html>
    <KCardGrid ...>
      ...
    </KCardGrid>
  </template>

 <template #javascript>
     export default { 
        ...
       }
  </template>
  
  <template #scss>
     .class {
        ...
      }
  </template>
</DocsExample>

I don't know when we will have it and I think it's fine to not be concerned as long as the approach take here is compatible enough so we can migrate it easily.

@EshaanAgg
Copy link
Contributor Author

Would you please clarify if and how we can override the code output, for example, how to achieve code sample outputs like here

Hi @MisRob! Using the new props introduced, we can't override the code sample as shown. As I see it, this should be used as an additional convenience helper: if we want to get the complete code for some components, we can migrate them to dedicated files and use the loadExample prop to fetch them dynamically.

In case of custom behavior, we can continue using the documentation components as we currently do:

<DocsShow>
  <DocsShowCode language='html'>
    <p>...partial code that we want to show </p>
  </DocsShowCode>
</DocsShowCode>

I'd like to be sure we don't lose the flexibility in this area.

Sure! That's why I have tried to only add some additional APIs, and not override any existing behaviour.

@MisRob
Copy link
Member

MisRob commented Jan 28, 2025

I see. Yeah I think it'd be fine, even though perhaps we can brainstorm a bit? Since this is something we do often. I think it wouldn't need to be complicated and quite aligned with your thinking and all work here, for example having

examples/example1.vue

<DocsExample>

</DocsExample>

and then loading it to the docs page

<DocsLoadExample name="example1">

All examples/ could be ignored by lint.

How would something like that sound to you?

@MisRob
Copy link
Member

MisRob commented Jan 28, 2025

Of course now the way to imagine example1 would be rather

<DocsShow/>
<DocsShowCode/>

Also I am not sure if we need another component DocsLoadExample just to load the example, perhaps there's more elegant way. I suggest it since it seems to be aligned with the approach you chose here.

I also want to keep thinking whether we can make all these perspectives work with auto-generation as you suggest originally in this PR. Every approach has benefits, and perhaps we can make it all work nicely together. Please let me know any thoughts.

@MisRob
Copy link
Member

MisRob commented Jan 28, 2025

I will return to this by the end of the week and try to think about these questions meanwhile too

@EshaanAgg
Copy link
Contributor Author

EshaanAgg commented Jan 28, 2025

Yes thank you, definitely. Basically, DocsExample's final interface is planned to look like

Yes! If the API contract for the same is not fixed, I would like for it to support three usages:

  1. Load and show the complete component from a file (complete code & example are shown to the user in a Vuetify-like experience):
<DocsExample component="/examples/KComponent/FileName" />
  1. Load the component from a file, but show specific code when navigating the code slots: This is useful when we have long components whose complete examples are present in separate files, but for the sake of brevity or clarity, we want the users to see only a part of it when using the documentation site. We can make use of named slots as you proposed for the same.
<DocsExample component="/examples/KComponent/FileName">
 <template #html>
    <KCardGrid ...>
      ...
    </KCardGrid>
  </template>

 <template #javascript>
     export default { 
        ...
       }
  </template>
  
  <template #scss>
     .class {
        ...
      }
  </template>
 </DocsExample>

In both the examples above, we can configure it to link to the complete example files through a GitHub link in the navigation bar. The slots here would have the highest priority.

  1. If we want to show some code to the user without explicitly linking to any file, we can skip the component prop altogether!
<DocsExample>
 <template #html>
    <KCardGrid ...>
      ...
    </KCardGrid>
  </template>

 <template #javascript>
     export default { 
        ...
       }
  </template>
 </DocsExample>

The output would be similar to (2), but no link to any GitHub documentation file.

@MisRob
Copy link
Member

MisRob commented Jan 28, 2025

Yes! If the API contract for the same is not fixed

Nono, no fixed :) I am opening this discussion because I am suspicious you may be interested in it and pretty confident we will come up with something nice :)

Thanks a lot for the comment, I will read it with fresh eyes on Thursday - I am just signing off and tomorrow am out for some errands.

@EshaanAgg
Copy link
Contributor Author

I see. Yeah I think it'd be fine, even though perhaps we can brainstorm a bit?

Most definitely :)

While designing this interface, my main idea was to place only complete SFCs as examples in the examples directory. Since we are assured that the examples in this directory are complete, we can dynamically load their template with props as described above. Keeping this in mind,

  1. I would prefer the example files not to have any Docs* tags present in them like:

examples/example1.vue

<DocsExample>

</DocsExample>

The same is more of a semantic choice for me. I would prefer the examples if they can be considered independent "drag & drop" SFCs that I can take inspiration from and use directly in my code from the GitHub files. Having these Doc* tags in them makes us aware of the internal implementation of the documentation and, in my opinion, defeats the purpose of moving the component to a separate file (as now the user can't simply copy and paste the same into their apps).

  1. All examples/ could be ignored by lint.

I would like to have linting enabled for them as well, as it would ensure that the code we showcase in the documentation is properly formatted and "idiomatic". We can perhaps fine-tune the lining configuration for the examples directory.

A simple example of the same can be found in this PR itself: we have the no-console-log rule configured in the KDS source code as it makes sense from the POV of a library, but when writing examples, it might be completely justified to use console.log to convey intended behavior.

methods: {
changeSortHandler(index, sortOrder) {
// eslint-disable-next-line no-console
console.log(`changeSort event emitted with index: ${index} and sortOrder: ${sortOrder}`);
},
},
};

It would make sense to turn this rule off for this directory to eliminate the eslint-disable comment. We can add more such rules as needed, but switching off all lining seems like a bad default to me.

Under typical usage, we would only need to disable linting when we provide custom display slots for the templates. In such cases, the developer can either:

  • Inline them in the template if the snippet is small
  • For each section of a documentation page of considerable length, create a separate component in a new file (with an eslint-disable for the whole file), then render that component in the documentation page itself. The component should live close to the documentation page (preferably in the same directory as the same).
  1. Also I am not sure if we need another component DocsLoadExample just to load the example.

I would also argue against it. I would prefer one component whose behavior I could customize to support all my use cases rather than having super-focused components for each related use case. The DocsShow component currently seems to be the best place to introduce it, though DocsExample is where it would be realized the best.

  1. I also want to keep thinking whether we can make all these perspectives work with auto-generation as you suggest originally in this PR.

Lost a bit of context here. Autogeneration as in that of the eslint-disable comments?

@MisRob
Copy link
Member

MisRob commented Jan 30, 2025

Hi @EshaanAgg, I really like the way you connected everything here and also the latest comment make sense. The interface you suggest is very well aligned with the experience we roughly imagined, and it feels very intuitive.

Regarding (3)

I would also argue against it. I would prefer one component whose behavior I could customize to support all my use cases rather than having super-focused components for each related use case. The DocsShow component currently seems to be the best place to introduce it, though DocsExample is where it would be realized the best.

The motivation for this was that if we did this (together with disabling lint and prettier for /examples), we wouldn't need

<!-- eslint-disable -->
<!-- prettier-ignore -->

even in cases we are showing the customized code sample via those #html, #javascript, and #scss slots, which for some pages is quite often.

However, I agree that it's cumbersome and harder to use, compared to your proposal. And besides other benefits you mention, I think having usual Vue components, without Docs.., in examples/ will scale better, e.g. for automated QA, or whatever we need to.

Since what we discuss here is meant to be an additional improvement, and I don't really have any concrete ideas how to further improve it, I think we can move on with this. The main thing is that we maintain the flexibility to override and this is satisfied. I discussed this work with the team recently and I believe it's aligned well with our thinking. Also, as soon as we have DocsExample, code samples won't be visible on first page load so if there's occasionally too much code, it won't be such a big deal like right now.

@MisRob
Copy link
Member

MisRob commented Jan 30, 2025

@EshaanAgg It's true this would be best done right in DocsExample. The feedback I left in #873 is non-blocking and I can coordinate with the contributor and ask them if it's fine we push them ourselves or they could resolve it in a follow-up PR. If it'd be helpful, let me know.

@MisRob
Copy link
Member

MisRob commented Jan 30, 2025

Also do you think you'd need to have #845 resolved? Since we're changing API here a bit, and that issue is not currently in progress, I don't mind coordinating some re-assignments if it makes this work easier. There's much more work coming in DocsExample waters that I can offer to other contributors instead.

You're welcome to suggest other strategies - I haven't yet looked into code so I am not sure what would make this smooth process.

@EshaanAgg
Copy link
Contributor Author

Thanks, @MisRob, for the kind comments! I do indeed feel that the best place to add this dynamic loading would be in the DocsExample component, as that is the principal component for which we finally intend to have this DX. I think the best way to proceed can be:

  • Merge Add DocsExample #873 into the repository as a proof of concept component of the UI & animated interactions that we want the component to have.
  • I can then continue the work there and configure the dynamic loading of the examples from SFCs as I have done in this PR, which is directly into the same.

After that, we can try rolling out the component on our documentation pages! I think that would be a great middle-ground where we can make sure that there in the most reuse of the work already done.

@MisRob
Copy link
Member

MisRob commented Jan 31, 2025

Great @EshaanAgg, thanks a lot. #873 is merged so feel free to build on top of it.

@EshaanAgg
Copy link
Contributor Author

Thanks a lot, @MisRob, for the heads up! I'll close this PR for now, and open a new PR as soon as possible!

@EshaanAgg EshaanAgg closed this Jan 31, 2025
@MisRob
Copy link
Member

MisRob commented Jan 31, 2025

Take your time @EshaanAgg

@EshaanAgg EshaanAgg deleted the docs branch February 3, 2025 19:31
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.

Configure prettier and eslint to treat DocsShowCode as code examples
2 participants