-
Notifications
You must be signed in to change notification settings - Fork 124
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
Add <tree> component for folder structure visualisations #1807
Conversation
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.
Very nicely done @tlylt 👍 Tried it out and so far it is working well :)
Just a quick review on the design and documentation first.
Seems like an issue with the way the tree processes the node? Notice that the If we try putting the tree code manually with the tooltip, it doesn't seem to have an issue. <div class="tree">
C:/course/
└── textbook/
└── <tooltip content="hello">index.md</tooltip>
</div> Not entirely sure what is causing this but somehow after the |
It seems like the problem of empty spaces added in front of the word 'index.md' is a separate issue. Is this a bug with the tooltip component? 🤔 |
Seems like the the usual However, since the I guess we can remove the white space generated from the |
Seems like this is indeed the issue. Curious how did you find that out 😅? I was looking over the code for the Tooltip/Trigger Vue components and couldn't find something similar to that effect. |
I noticed the white space by looking at the HTML code output on the browser. Since the |
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.
Other than the tooltip
/trigger
issue, I think this PR is good to merge.
Might also want to add a note in the documentation to show that tooltip
/trigger
is not supported in the tree
component yet and open a new issue tracker for this.
EDIT: There seems to be some merge conflict as well.
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.
Thanks @tlylt!
LGTM 👍
} | ||
node.name = 'div'; | ||
node.attribs.class = node.attribs.class ? `${node.attribs.class} tree` : 'tree'; | ||
node.children[0].data = TreeNode.visualize(node.children[0].data); |
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.
DomParser nodes returned by htmlparser have various type
s, with the isSpecial
option specified the (only) child is treated as text (much like script/style tags).
You could use cheerio apis here instead to attach the visualized string as html, which solves the main (non-whitespace) issue with #1831.
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.
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.
You could use cheerio apis here instead to attach the visualized string as html
Do you mean changing it to something like:
processNode: (pluginContext, node) => {
if (node.name !== 'tree') {
return;
}
cheerio(node).text(TreeNode.visualize(cheerio(node).text()));
cheerio(node).addClass('tree');
node.name = 'div';
},
I tried this but doesn't seem to resolve the issue with tooltips (still has white space + does not show up on hover).
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.
yup, but this is the wrong method .text(
(still attaching as text, not html)
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.
reping in case this was missed @tlylt.
though not urgent, we can put up an issue if no time to test it yet.
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.
reping in case this was missed @tlylt. though not urgent, we can put up an issue if no time to test it yet.
Thanks for the reminder. I have tested and setting it via .html (iirc) does achieve the effect as you mentioned. This resolves the white-space issue but triggers are still quite buggy/ not functional. I have #1831 to keep track of this and was thinking to investigate along with the comment #1831 (comment) before pushing a fix.
What is the purpose of this pull request?
Overview of changes:
Fixes #1660
Provides a convenient way to create lightweight tree-like textual output, suitable for folder structure visualisations.
Syntax:
To
More details in UG's tree section
A summary of the work done:
core/src/plugins/default/markbind-plugin-tree.js
core/test/unit/plugins/default/tree.test.js
packages/cli/test/functional/test_site/testTree.md
Anything you'd like to highlight / discuss:
Testing the above code will give:
Notice the white space added in front of the words "index.md" and "via trigger". While the direct tooltip approach does not work on hover, the trigger approach works. Not too sure if this is a bug with the tooltip/trigger element or is it due to the newly added tree component.
Testing instructions:
Tests cases are added in
packages/cli/test/functional/test_site/testTree.md
packages/core/test/unit/plugins/default/tree.test.js
User guide section
Proposed commit message: (wrap lines at 72 characters)
Add component for folder structure visualisations
It is tedious to manually create tree-like structure diagrams.
Let's add a component to conveniently convert
an indented list of items into tree-like textual output.
This is especially useful for file system visualisations.
Checklist: ☑️