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

[Progress indicator]: Add viewBox attr to svg to allow scaling #5086

Closed
annawen1 opened this issue Jan 17, 2020 · 4 comments · Fixed by #5122
Closed

[Progress indicator]: Add viewBox attr to svg to allow scaling #5086

annawen1 opened this issue Jan 17, 2020 · 4 comments · Fixed by #5122

Comments

@annawen1
Copy link
Member

annawen1 commented Jan 17, 2020

Summary

We are bumping up icon sizes to 20px by 20px for the expressive theme changes. Ticket here: carbon-design-system/carbon-for-ibm-dotcom#1039

However the some svgs for the progress indicator component don't scale correctly.

Screen Shot 2020-01-16 at 10 47 26 AM

Looking at the code it seems that there are no attributes for the svg elements

if (current) {
      return (
        <svg>
          <path d="M 7, 7 m -7, 0 a 7,7 0 1,0 14,0 a 7,7 0 1,0 -14,0" />
          <title>{description}</title>
        </svg>
      );
    }
    
     ....

    return (
      <svg>
        <title>{description}</title>
        <path d="M8 1C4.1 1 1 4.1 1 8s3.1 7 7 7 7-3.1 7-7-3.1-7-7-7zm0 13c-3.3 0-6-2.7-6-6s2.7-6 6-6 6 2.7 6 6-2.7 6-6 6z" />
      </svg>

If the viewBox attribute is added to the svg elements, that would allow for the current and incomplete indicator icons to scale to 20x20.

Justification

This is needed for the expressive theme changes by the DDS team.

"Must have" functionality

  • svgs should have viewBox attribute defined

Specific timeline issues / requests

Do you want this work within a specific time period? Is it related to an
upcoming release?

It would be great to have this in during our next sprint (Sprint 20-02) which ends on January 31st.

NB: The Carbon team will try to work with your timeline, but it's not
guaranteed. The earlier you make a request in advance of a desired delivery
date, the better!

@asudoh
Copy link
Contributor

asudoh commented Jan 18, 2020

Beginning with v10, Carbon icons library provide ones for various sizes. So if you want a difference size having an API to render another icon seems to help you. If you search for renderIcon in the codebase, you'll find how such kind of API is implemented for other components in Carbon library.

@annawen1
Copy link
Member Author

annawen1 commented Jan 20, 2020

@asudoh Thanks for getting back to me! When I search renderIcon in the codebase I see it's a prop that a lot of the components take in. Issue with the Progress Indicator is that the icons I'm trying to change are hardcoded and not dependent on the renderIcon prop or the icons API.

With the expressive theme we currently are applying scss styles when an expressive flag is turned on. For example:

@mixin combo-box-expressive {
  .#{$prefix}--list-box__invalid-icon,
  .#{$prefix}--list-box__selection svg {
    width: carbon--rem(20px);
    height: carbon--rem(20px);
  }
}

This would change the icon size for the combo-box via scss.

We're not tampering with the carbon components too much, mostly the styles and props being passed in but not the actual code of the components. All I need for the above to work properly is for the viewBox attribute to be set for those hardcoded svg icons.

@asudoh
Copy link
Contributor

asudoh commented Jan 21, 2020

When I search renderIcon in the codebase I see it's a prop that a lot of the components take in. Issue with the Progress Indicator is that the icons I'm trying to change are hardcoded and not dependent on the renderIcon prop or the icons API.

You are right, my suggestion is implementing renderIcon prop support, taking a cue from other components. A PR will be nice!

@annawen1
Copy link
Member Author

@asudoh Ah gotcha, ok PR coming up! I'll link it to this issue, thanks!

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.

2 participants