Skip to content
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

starkSvgViewBox directive doesn't update viewBox of custom SVG already containing viewbox #1216

Closed
Dedepon opened this issue Mar 25, 2019 · 5 comments · Fixed by #1254
Closed
Assignees
Milestone

Comments

@Dedepon
Copy link
Contributor

Dedepon commented Mar 25, 2019

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[X] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/NationalBankBelgium/stark/blob/master/CONTRIBUTING.md#got-a-question-or-problem

Current behavior

When using the starkSvgViewBox directive on a custom svg, if that svg already has a viewBox set, the directive doesn't change it.

Expected behavior

The viewBox should be changed to the configured one.

Minimal reproduction of the problem with instructions

In any component, use a mat-icon with a custom svg file (which should be imported).
In the mat-icon component, add the starkSvgViewBox directive and change the default value.

What is the motivation / use case for changing the behavior?

Resize the icons to fit the design.

Environment


Angular version: 7.x
Stark version: 10.0.0-beta.6

Browser:
- [X] Chrome (desktop) version 56
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX

@Dedepon
Copy link
Contributor Author

Dedepon commented Mar 25, 2019

Here's the icon :
icon-upload.zip

@christophercr
Copy link
Collaborator

@dede-pon in fact, if an SVG already has a viewBox set then it is resizable. The purpose of the starkSvgViewBox is to add the viewBox only if the SVG does not have such attribute (otherwise it cannot be resized via CSS).

I tested on my side and I can confirm that the icon you provide can be resized just with CSS as follows:

/* 
    make sure to add the 'mat-icon' in the selector otherwise the 
    default 'mat-icon' rule from Angular Material takes precedence
*/
.mat-icon.some-class {  /* or  mat-icon.some-class also works */
  height: 120px;
  width: 120px;
}

Could you please double check if this works for you?

@Dedepon
Copy link
Contributor Author

Dedepon commented Apr 12, 2019

Hello,

I confirm that you can resize the icon using CSS.

I just pointed out that the svg's initial viewBox isn't changed. If you consider it to be a normal behavior, you can close the issue ;-).

@christophercr
Copy link
Collaborator

christophercr commented Apr 12, 2019

Well, I don't have any clear opinion about this. What could be the reason to override the initial viewBox of an SVG?

What do you think @cnomes?

@carlo-nomes carlo-nomes self-assigned this Apr 15, 2019
carlo-nomes added a commit to carlo-nomes/stark that referenced this issue Apr 15, 2019
carlo-nomes added a commit to carlo-nomes/stark that referenced this issue Apr 15, 2019
…irective

  - updated test

ISSUES CLOSED: NationalBankBelgium#1216

BREAKING CHANGES: `starkSvgViewBox` will now overwrite the `viewBox` value of the svg. If this is not desired the `starkSvgViewBox` directive should be removed from the element.
@carlo-nomes
Copy link
Collaborator

carlo-nomes commented Apr 15, 2019

@dede-pon I would suggest not using this directive on the icon you provided since it already has (a correct) viewBox set. But I did change the functionality of starkSvgViewBox to overwrite the viewBox of an svg.

carlo-nomes added a commit to carlo-nomes/stark that referenced this issue Apr 15, 2019
…irective

  - updated test

ISSUES CLOSED: NationalBankBelgium#1216

BREAKING CHANGES: `starkSvgViewBox` will now overwrite the `viewBox` value of the svg. If this is not desired the `starkSvgViewBox` directive should be removed from the element.
carlo-nomes added a commit to carlo-nomes/stark that referenced this issue Apr 15, 2019
…irective

  - updated test

ISSUES CLOSED: NationalBankBelgium#1216

BREAKING CHANGES: `starkSvgViewBox` will now overwrite the `viewBox` value of the svg. If this is not desired the `starkSvgViewBox` directive should be removed from the element.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants