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: CHARTS-6059 Extract common text from SDK guides #81

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

lilyh4
Copy link

@lilyh4 lilyh4 commented Mar 30, 2023

Summary

  • Our SDK examples share a lot of common text in their README.md files
  • Common partial files have been added to a new directory examples/docs
  • Used npm package markdown-include to support building our README files with other Markdown files
    • With each outputted README.md (one per SDK example) there now needs to be a new markdown.json file which specifies the build file for it
      • These build files are located in examples/.md-config
    • Until we set up an action, compilation is manual (see below)
  • With this approach, future text changes should only be made to the configuration files or the partial docs

Script to compile all SDK guide README files:

for i in examples/charts/* examples/dashboard/*;
  do ./node_modules/markdown-include/bin/cli.js "${i}"/markdown.json;
done;

Updated file tree

charts-embed-sdk/examples
├── .md-config
│   ├── chart
|	|   ├── authenticated-custom-jwt.md 	
|	|   └── ...other md config files 			
|	└── dashboard
│	    └── ...other md config files 		
├── charts
│   ├── authenticated-custom-jwt
|	|   ├── assets
|	|   ├── src 	
|	|   ├── app.js 	
|	|   ├── index.html 
|	|   ├── markdown.json 		
|	|   ├── package.json 	
|	|   ├── package.json 	
|	|   └── package-lock.json 
|	└── ...other examples	
├── dashboard
|	└── ...other examples	
├── docs
│   ├── chart
|	|   └── ...partial md files
|	├── dashboard
|	|   └── ...partial md files
|	└── ...partial md files
└── README.md

…n each README.md

chore: Fix use-own-data/ minor nits and remove extra desc in click event guides

chore: Indendation nits and remove unused partial file

chore: Remove /click-events partial files and misc fixes
@lilyh4 lilyh4 added documentation Improvements or additions to documentation enhancement New feature or request labels Mar 30, 2023
@lilyh4 lilyh4 self-assigned this Mar 30, 2023
Copy link
Collaborator

@khanguslee khanguslee left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up the README files in this repository, I know there was a lot of duplicate text because I am guilty for being responsible for this. This change will make maintaining documentation a lot easier until we move to a different solution. Some things that are missing in this PR include:

A bash script to call your manual script:

for i in examples/charts/* examples/dashboard/*;
  do ./node_modules/markdown-include/bin/cli.js "${i}"/markdown.json;
done;

and then call this bash script using something like yarn build:markdown.

Additionally, we should add the steps to build the markdown somewhere. Maybe in a DEVELOPMENT.md file or another section in the main README.md?

@@ -0,0 +1 @@
Happy Charting! 🚀 📈
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move the docs folder into the .md-config directory? I don't want to have additional files/folders alongside the examples folder. As long as we have everything inside one place, we can dedicate documentation to the .md-config

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with Angus, it would be great if all config stuff is in one place separately from the example guides.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks guys, I've renamed the .md-config folder to .markdown and relocated the partial docs files under it in f119aff

examples/.md-config/chart/authenticated-custom-jwt.md Outdated Show resolved Hide resolved
Comment on lines 3 to 12
#include "examples/docs/chart/background/start-block.md"

#include "examples/docs/chart/background/desc-simple.md"

#include "examples/docs/chart/background/desc-extended.md"

This sample shows how to use the JavaScript Embedding SDK to render **authenticated** embedded charts, specifically via a custom jwt authentication. The sample app has its own authentication and token issuing logic. You may want to follow a similar approach if you are not integrating with an external authentication mechanism or your authentication is not based on JWTs. Alternatively, if you are using an existing authentication mechanism that issues JWTs, you can use those tokens to authenticate your chart rendering requests after configuring a Charts authentication provider that can validate the JWT.

#include "examples/docs/chart/background/features-authenticated.md"
- 🔑 Custom JWT authentication via `jsonwebtoken`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I almost feel like we're refactoring too much. I would actually be fine if we had the titles not refactored so that these markdown files show the general layout. It wasn't immediately apparent that the way you structured the folders was how you structured the refactoring.

I would assume that the authenticated and unauthenticated readmes should have the same description (authenticated/unauthenticated), then we can add on afterwards.

# MongoDB Charts Embedding Example - Authenticated Embedded Chart

## Background

#include "examples/docs/chart/authenticated-background.md"

Extra text goes here...

## Features

#include "examples/docs/chart/authenticated-features.md"
- Extra features goes here...

Happy otherwise, but would be interested to hear what you think!

Copy link
Author

Choose a reason for hiding this comment

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

Definitely see your point with keeping the titles to aid readability. I've updated this in two fairly larger commits, one that cleans up the dashboard guides (945518e) and the other that cleans up the chart guides (ccf7cb6)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we can have one file with an array of all such objects and that file is in the md-config so that the example guides themselves don't contain config md files?

Copy link
Author

@lilyh4 lilyh4 May 8, 2023

Choose a reason for hiding this comment

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

Based on the way the npm package works, creating the array of objects will be little more involved so I've decided not to implement that within the scope of this ticket. However, I've moved the all the build files to a new .markdown/compilation directory for now in 69b70c6 so the example guides don't contain them.


10. **Optional**
11. **Optional**
In the same menu, note the **User Specified Filters** option. If you wish to try out filtering on your own dataset, you will need to whitelist a field by which to filter on. For example, our sample AirBnB dataset filters on `address.country`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: While we are here, can we change the "whitelist" to "allowed list" to reflect the current product state? This can be done separately too.

Copy link
Collaborator

@kristinamongo kristinamongo left a comment

Choose a reason for hiding this comment

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

Wow, thank you Lily for the amazing effort with the example guides, this was a lot of work.
You also caught a lot of inaccuracies in the examples themselves which just proves how needed this refactoring was. Well done!

I would suggest some quick changes around the structure and naming of the partial sections:
These partial section file names should be unique and more obvious in what they contain by their names and the names should reflect the logical meaning of the content more than the literal text inside the files. The naming is very important here because of the files discoverability with a quick glance. If the section file names are not clear, the whole structure might become a bit confusing to work with. I’m sure this is more easily said than done but here are some examples:

  • The start-block does not speak much to me. It will be more clear if it’s called "mdb-charts-docs-link" or something similar. Also, there are two different files start-block with different purpose in two different folders, so I need to open in order to understand what’s the content.
  • Same for the dashboard background-title - it will be more discoverable as "mdb-dashboard-docs-link".
  • Also there should be more consistency in the names - some of them have "desc" at the beginning, some at the end, plus writing the whole "description" word would be better in my opinion.

Maybe more generalisation is needed in the guides so they can be moved out into bigger chunks files to make them easier to construct without compromising quality. A bit of reordering can achieve this. For example, I think "use your own data" can be one block in a lot of the examples instead of start, replace and end blocks - I do realise that might not be possible everywhere so it needs to be checked case by case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants