-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiMarkdownEditor] Added popover to display help syntax text when no custom plugins are available #5147
[EuiMarkdownEditor] Added popover to display help syntax text when no custom plugins are available #5147
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_5147/ |
🚀 great example idea; I've created a PR against your branch with an easier way to disable the tooltip plugin (and we can extend it with any other complete plugins in the future) |
…ugins-ui-excludes Made opting out of the tooltip markdown plugin much easier
…help-no-plugins-ui
…help-no-plugins-ui
…help-no-plugins-ui
…help-no-plugins-ui
@chandlerprall the CI is always failling 🤷🏽 with: 15:25:16 Generating typescript definitions file
15:25:16 eui.d.ts(11728,29): error TS2307: Cannot find module 'unified/types/ts3.4/' or its corresponding type declarations.
15:26:16 eui.d.ts(11729,34): error TS2307: Cannot find module 'unified/types/ts3.4/' or its corresponding type declarations.
15:26:16 eui.d.ts(11729,377): error TS2307: Cannot find module 'unified/types/ts3.4/' or its corresponding type declarations.
15:26:16 child_process.js:669
15:26:16 throw err; |
👉 elizabetdev#24 will fix the CI failure |
…ns-ui `unified` types
Thanks @thompsongl! 😄 |
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5147/ |
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 like the simplicity of just showing the popover when there's not much more context than just pointing to GH markdown docs. 💯
The component fixes mostly LGTM (with a few fixes mentioned), but most of my comments revolve around the docs to try to make it clearer / get straight to the point.
{isAstShowing ? 'Hide editor AST' : 'Show editor AST'} | ||
</EuiButton> | ||
</div> | ||
{isAstShowing && <EuiCodeBlock language="json">{ast}</EuiCodeBlock>} |
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.
Can be in a follow up PR:
Because this continues to be added to every examples, and I can understand the nicety to it, I start worrying that it actually looks like it comes with the component. Can we rethink a way to render this. Like maybe look at how the Page examples are wrapped in a custom component but only renders what it needed for the consumer.
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'll follow up on this. 👍🏽
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_5147/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5147/ |
@cchaos I finished the changes based on your review:
|
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, @miukimiu!
Preview documentation changes for this PR: https://eui.elastic.co/pr_5147/ |
…help-no-plugins-ui
Preview documentation changes for this PR: https://eui.elastic.co/pr_5147/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5147/ |
4c767b9
to
92dadbc
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_5147/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5147/ |
@cchaos and @thompsongl these are the new changes:
|
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 UI looks great. But I did find a possible edge case where if there are plugins but none of the plugins actually have helpText
.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5147/ |
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.
👍 Double-checked the different examples with the new logic. LGTM!
Rebase and CL update needed.
Thanks, @cchaos! I confirmed and it's updated and rebased. |
Summary
Closes #5127.
Design
If we're not showing syntax for plugins, we display the help syntax in a popover. No need to have a modal just with a link to the GitHub Flavored Markdown page.
The Markdown button was not displaying correctly on hover.
New docs section
As per my comment on #5127 (comment). I think it isn't easy to know how to unregister plugins. For me, it makes sense to have a section in our docs to show how to do it. So I created the following section on
#/editors-syntax/markdown-editor#unregistering-plugins
.Checklist
[ ] Checked in mobile[ ] Props have proper autodocs and playground toggles[ ] Checked Code Sandbox works for the any docs examples[ ] Checked for breaking changes and labeled appropriately[ ] Checked for accessibility including keyboard-only and screenreader modes