Octicons: support icon-size property

[hcwong]

What is the purpose of this pull request?
• [x] Bug fix

Fixes#1096

What changes did you make? (Give an overview)
The bug occured because there was the style property currently for icons only works for favicons. We do some HTML manipulation in the parser to detect if theres magnification and adjust the size accordingly. We cannot do this in the vue component as to change a svg's size, we need to change the element directly and its surrounding divs.

Is there anything you'd like reviewers to focus on?
I haven't really played with the parser code, so if its not in line with the current style, do let me know. Also, I'm not sure if this octicon icon with icon-size is used anywhere else. If so, I might want to generalize this solution.

Testing instructions:
Run the code with octicons and use the icon-size property. Regardless the icon-size property is there or not, it should not break the code and the octicon should vary in size

Proposed commit message: (wrap lines at 72 characters)
Support icon-size property for octicons.

[hcwong]

@le0tan can I tag you as reviewer haha?

[dvrylc]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

[hcwong]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

Not sure about this, but do we need to modify the svg height and width attribute directly to change the actual dimensions of the svg?

[dvrylc]

Not sure about this, but do we need to modify the svg height and width attribute directly?

Not quite sure what you mean but the .octicon class is set on the SVG element, so by modifying the class attributes we're setting the SVG's height and width. Was that your question?

[hcwong]

What is the purpose of this pull request?
• [x] Bug fix

Fixes#1096

What changes did you make? (Give an overview)
The bug occured because there was the style property currently for icons only works for favicons. We do some HTML manipulation in the parser to detect if theres magnification and adjust the size accordingly. We cannot do this in the vue component as to change a svg's size, we need to change the element directly and its surrounding divs.

Is there anything you'd like reviewers to focus on?
I haven't really played with the parser code, so if its not in line with the current style, do let me know. Also, I'm not sure if this octicon icon with icon-size is used anywhere else. If so, I might want to generalize this solution.

Testing instructions:
Run the code with octicons and use the icon-size property. Regardless the icon-size property is there or not, it should not break the code and the octicon should vary in size

Proposed commit message: (wrap lines at 72 characters)
Support icon-size property for octicons.

[hcwong]

@le0tan can I tag you as reviewer haha?

[dvrylc]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

[hcwong]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

Not sure about this, but do we need to modify the svg height and width attribute directly to change the actual dimensions of the svg?

[dvrylc]

Not sure about this, but do we need to modify the svg height and width attribute directly?

Not quite sure what you mean but the .octicon class is set on the SVG element, so by modifying the class attributes we're setting the SVG's height and width. Was that your question?

[hcwong]

What is the purpose of this pull request?
• [x] Bug fix

Fixes#1096

What changes did you make? (Give an overview)
The bug occured because there was the style property currently for icons only works for favicons. We do some HTML manipulation in the parser to detect if theres magnification and adjust the size accordingly. We cannot do this in the vue component as to change a svg's size, we need to change the element directly and its surrounding divs.

Is there anything you'd like reviewers to focus on?
I haven't really played with the parser code, so if its not in line with the current style, do let me know. Also, I'm not sure if this octicon icon with icon-size is used anywhere else. If so, I might want to generalize this solution.

Testing instructions:
Run the code with octicons and use the icon-size property. Regardless the icon-size property is there or not, it should not break the code and the octicon should vary in size

Proposed commit message: (wrap lines at 72 characters)
Support icon-size property for octicons.

[hcwong]

@le0tan can I tag you as reviewer haha?

[dvrylc]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

[hcwong]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

Not sure about this, but do we need to modify the svg height and width attribute directly to change the actual dimensions of the svg?

[dvrylc]

Not sure about this, but do we need to modify the svg height and width attribute directly?

Not quite sure what you mean but the .octicon class is set on the SVG element, so by modifying the class attributes we're setting the SVG's height and width. Was that your question?

[hcwong]

What is the purpose of this pull request?
• [x] Bug fix

Fixes#1096

What changes did you make? (Give an overview)
The bug occured because there was the style property currently for icons only works for favicons. We do some HTML manipulation in the parser to detect if theres magnification and adjust the size accordingly. We cannot do this in the vue component as to change a svg's size, we need to change the element directly and its surrounding divs.

Is there anything you'd like reviewers to focus on?
I haven't really played with the parser code, so if its not in line with the current style, do let me know. Also, I'm not sure if this octicon icon with icon-size is used anywhere else. If so, I might want to generalize this solution.

Testing instructions:
Run the code with octicons and use the icon-size property. Regardless the icon-size property is there or not, it should not break the code and the octicon should vary in size

Proposed commit message: (wrap lines at 72 characters)
Support icon-size property for octicons.

[hcwong]

@le0tan can I tag you as reviewer haha?

[dvrylc]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

[hcwong]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

Not sure about this, but do we need to modify the svg height and width attribute directly to change the actual dimensions of the svg?

[dvrylc]

Not sure about this, but do we need to modify the svg height and width attribute directly?

Not quite sure what you mean but the .octicon class is set on the SVG element, so by modifying the class attributes we're setting the SVG's height and width. Was that your question?

[hcwong]

What is the purpose of this pull request?
• [x] Bug fix

Fixes#1096

What changes did you make? (Give an overview)
The bug occured because there was the style property currently for icons only works for favicons. We do some HTML manipulation in the parser to detect if theres magnification and adjust the size accordingly. We cannot do this in the vue component as to change a svg's size, we need to change the element directly and its surrounding divs.

Is there anything you'd like reviewers to focus on?
I haven't really played with the parser code, so if its not in line with the current style, do let me know. Also, I'm not sure if this octicon icon with icon-size is used anywhere else. If so, I might want to generalize this solution.

Testing instructions:
Run the code with octicons and use the icon-size property. Regardless the icon-size property is there or not, it should not break the code and the octicon should vary in size

Proposed commit message: (wrap lines at 72 characters)
Support icon-size property for octicons.

[hcwong]

@le0tan can I tag you as reviewer haha?

[dvrylc]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

[hcwong]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

Not sure about this, but do we need to modify the svg height and width attribute directly to change the actual dimensions of the svg?

[dvrylc]

Not sure about this, but do we need to modify the svg height and width attribute directly?

Not quite sure what you mean but the .octicon class is set on the SVG element, so by modifying the class attributes we're setting the SVG's height and width. Was that your question?

[hcwong]

What is the purpose of this pull request?
• [x] Bug fix

Fixes#1096

What changes did you make? (Give an overview)
The bug occured because there was the style property currently for icons only works for favicons. We do some HTML manipulation in the parser to detect if theres magnification and adjust the size accordingly. We cannot do this in the vue component as to change a svg's size, we need to change the element directly and its surrounding divs.

Is there anything you'd like reviewers to focus on?
I haven't really played with the parser code, so if its not in line with the current style, do let me know. Also, I'm not sure if this octicon icon with icon-size is used anywhere else. If so, I might want to generalize this solution.

Testing instructions:
Run the code with octicons and use the icon-size property. Regardless the icon-size property is there or not, it should not break the code and the octicon should vary in size

Proposed commit message: (wrap lines at 72 characters)
Support icon-size property for octicons.

[hcwong]

@le0tan can I tag you as reviewer haha?

[dvrylc]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

[hcwong]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

Not sure about this, but do we need to modify the svg height and width attribute directly to change the actual dimensions of the svg?

[dvrylc]

Not sure about this, but do we need to modify the svg height and width attribute directly?

Not quite sure what you mean but the .octicon class is set on the SVG element, so by modifying the class attributes we're setting the SVG's height and width. Was that your question?

[hcwong]

What is the purpose of this pull request?
• [x] Bug fix

Fixes#1096

What changes did you make? (Give an overview)
The bug occured because there was the style property currently for icons only works for favicons. We do some HTML manipulation in the parser to detect if theres magnification and adjust the size accordingly. We cannot do this in the vue component as to change a svg's size, we need to change the element directly and its surrounding divs.

Is there anything you'd like reviewers to focus on?
I haven't really played with the parser code, so if its not in line with the current style, do let me know. Also, I'm not sure if this octicon icon with icon-size is used anywhere else. If so, I might want to generalize this solution.

Testing instructions:
Run the code with octicons and use the icon-size property. Regardless the icon-size property is there or not, it should not break the code and the octicon should vary in size

Proposed commit message: (wrap lines at 72 characters)
Support icon-size property for octicons.

[hcwong]

@le0tan can I tag you as reviewer haha?

[dvrylc]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

[hcwong]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

Not sure about this, but do we need to modify the svg height and width attribute directly to change the actual dimensions of the svg?

[dvrylc]

Not sure about this, but do we need to modify the svg height and width attribute directly?

Not quite sure what you mean but the .octicon class is set on the SVG element, so by modifying the class attributes we're setting the SVG's height and width. Was that your question?

[hcwong]

What is the purpose of this pull request?
• [x] Bug fix

Fixes#1096

What changes did you make? (Give an overview)
The bug occured because there was the style property currently for icons only works for favicons. We do some HTML manipulation in the parser to detect if theres magnification and adjust the size accordingly. We cannot do this in the vue component as to change a svg's size, we need to change the element directly and its surrounding divs.

Is there anything you'd like reviewers to focus on?
I haven't really played with the parser code, so if its not in line with the current style, do let me know. Also, I'm not sure if this octicon icon with icon-size is used anywhere else. If so, I might want to generalize this solution.

Testing instructions:
Run the code with octicons and use the icon-size property. Regardless the icon-size property is there or not, it should not break the code and the octicon should vary in size

Proposed commit message: (wrap lines at 72 characters)
Support icon-size property for octicons.

[hcwong]

@le0tan can I tag you as reviewer haha?

[dvrylc]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

[hcwong]

@hcwong I actually did a bit of digging on this problem (before I realized you were working on it) and found this:

The way icon-size currently works is that it sets classes like fa-2x (for example) on the icon elements. Font Awesome's CSS then provides the styles for these classes by setting the font-size property on them. This approach doesn't work for octicons because their size is controlled by height and width (as you already know) and I managed to fix this by writing a bit of CSS: ef4f0b5.

Not sure if this is useful to you, but just leaving it here as an idea.

Not sure about this, but do we need to modify the svg height and width attribute directly to change the actual dimensions of the svg?

[dvrylc]

Not sure about this, but do we need to modify the svg height and width attribute directly?

Not quite sure what you mean but the .octicon class is set on the SVG element, so by modifying the class attributes we're setting the SVG's height and width. Was that your question?