-
Notifications
You must be signed in to change notification settings - Fork 30
Icons #1413
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
Icons #1413
Conversation
Co-authored-by: Fabrizio Ferri-Benedetti <fabri.ferribenedetti@elastic.co>
Co-authored-by: Fabrizio Ferri-Benedetti <fabri.ferribenedetti@elastic.co>
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.
It'd be great if we can get :icon:
to work to match Github flavored markdown.
I can take a stab at that in a follow up if needed?
Otherwise we need a clear new syntax i:<icon>:
is not quite it!
Other than that love this!
I got it working. Let me know what you think. The main change is in the But I'm actually worried, this will have other unintended sideeffects. E.g. if a directive option has the same name as an icon. IMO, the safest option is another syntax. Looking at the Myst documentation the most logical syntax would be:
|
# Conflicts: # src/Elastic.Markdown/Myst/Renderers/SectionedHeadingRenderer.cs
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.
(Not an issue in the PR, but in GH) What a weird false positive... If you open this in a hex editor, U+2009 (0xE2 0x80 0x89
) is nowhere to be seen.
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.
Ah, you mean the error in the failing test. I was not able to find a difference and I don't think this error will cause any troubles in production.
We might feel the pain when we add tests in the future 😅
But I think the simplified test is testing the necessary parts. Which is fine.
Thank you for checking.
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.
If you're referring to the docs-build hints, it's not a false positive, the character really is invisible. It should be a space or not a space. Easy to catch with a regex such as \u2009
if I remember well. I fixed most of those in another PR, so a rebase should do the trick.
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.
No. I had a test that was failing only in windows.
The error said that the output doesn't match the expected. But it was exactly the same.
Hence, @cotti suspected that there must be a weird space or something.
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.
Ah sorry, I actually misunderstood this haha :D
public class IconRoleHtmlRenderer : HtmlObjectRenderer<IconsRole> | ||
{ | ||
|
||
[SuppressMessage("Reliability", "CA2012:Use ValueTasks correctly")] |
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 think the Suppress is not needed anymore here
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.
Oh. Thanks!
The copy-pasta syndrome (:
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.
🚂
Closes #409
Changes
{icon}`icon-name`
Notes
Using the syntax:icon-name:
did not fully work as expected. Especially, in cases where the icon would be the first element in a list or other v3 markdown items.I suspect it's conflicting with the
:::{}
blocks and:option:
syntax.Future improvements
Somehow automatically copy the files from https://github.com/elastic/eui/tree/main/packages/eui/src/components/icon/svgs during the build process.
Preview
https://docs-v3-preview.elastic.dev/elastic/docs-builder/pull/1413/syntax/icons