Uh oh!
There was an error while loading. Please reload this page.
Incorrect octicon name crashes live preview - #1377
Conversation
ang-zeyu
left a comment
There was a problem hiding this comment.
Looks good!
Let's remove the unrelated style changes and keep that to a separate pr (there's quite a few files in the markdown-it folder that should really be linted) (some not, to preserve the style of the external source we extracted it from)
KendrickAng
commented
Oct 18, 2020
Got it! In the meantime, what do you think about extending octicon functionality to allow configuration of height and width of octicons with markdown? I mentioned in the description that This might be beneficial for users who want to manually adjust octicon width/height with minimal hassle |
hmm definitely nice to have, although icons are already responsive to If we support sizing for some icons but not others it may be slightly inconsistent feature wise though. Could get user opinions from @damithc too |
| if (!octicons.hasOwnProperty(iconFontName)) { | ||
| return `<span aria-hidden="true"></span>`; | ||
| } | ||
| } |
There was a problem hiding this comment.
is it possible to combine the below checks (iconFontType === 'octicon' and `iconFontType === 'octiconlight') and only adjust the style conditionally?
so we have less repeated code down there, and integrate this check only where its needed
There was a problem hiding this comment.
Right, that would be much better, I'll do that
damithc
commented
Oct 19, 2020
As @ang-zeyu mentioned, if we were to add that feature, we should do that for all three types of icons. Perhaps no need as we can use the font size to adjust the icon size? |
My concern was that users not familiar with css would have trouble with the usage of classes to resize the icons (I was unaware that you could use For my own learning: what is the difference using |
damithc
commented
Oct 20, 2020
From what I understand (I could be wrong), these icons are implemented as a text font. So, they can be manipulated as text. |
| ? octicons[iconFontName].toSVG({"class": iconClass}) | ||
| : octicons[iconFontName].toSVG(); | ||
| // ensure octicons are valid | ||
| if (!octicons.hasOwnProperty(iconFontName)) { |
There was a problem hiding this comment.
should this be above octicons[iconFontName]? 😮
There was a problem hiding this comment.
No, it shouldn't ... so sorry about that 😓
ang-zeyu
commented
Oct 24, 2020
Lgtm 👍
My view is that it may still be convenient to be able to easily adjust just the size of the icon Perhaps we could raise this as a low priority issue? But if such cases are few and far between then maybe not worth the maintenance costs
and any element that uses |
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Bug fix
What is the rationale for this request?
Resolves#1266
Currently, if a user tries to use an invalid octicon, the live server crashes. On the other hand, this does not happen with glyphicons since usage of empty glyphicons just results in an empty
<span glyphicon glyphicon-invalid></span>.What changes did you make? (Give an overview)
Added an existence check for the octicon the user wants to use, returning an empty
<span>otherwise.Provide some example code that this change will affect:
Is there anything you'd like reviewers to focus on?
https://primer.style/octicons/packages/javascript
It is possible for us to use the octicons API to adjust the height/width of the glyphicons we need. This would be much less of a hassle that currently having to create a new class just to resize the icon. Perhaps we could consider adding in this functionality?
Testing instructions:
Try serving a webpage with invalid octicon.
Proposed commit message: (wrap lines at 72 characters)
add validity check for octicons