Templarian / MaterialDesign-React

Dist for Material Design Icons React Component for JS/TypeScript
https://materialdesignicons.com
MIT License
142 stars 20 forks source link

Make the default `size` be equal to 1, not null #16

Closed mririgoyen closed 4 years ago

mririgoyen commented 5 years ago

Please make the size prop default to 1 if no value is provided. If you leave size off, the icon simply will not render (0 viewbox).

In most cases,

<Icon path={mdiMenu} />

is preferable to

<Icon path={mdiMenu} size={1} />
Templarian commented 5 years ago

Most are styling the icons with CSS/SCSS, so adding a default width and height would not be ideal.

Is there a reason you couldn't just set the default size with CSS? Usually setting the default fill color also.

mririgoyen commented 5 years ago

It seems extremely silly to have to specify widths for every use of every icon. I want to use an icon, and I expect it to appear when I put it in my code. If I want to further style that icon, I can then choose to add the optional size prop or use CSS.

Currently, a page with 15+ icons on it either has to look like:

<Icon path={mdiMenu} size={1} />
<Icon path={mdiAccount} size={1} />
<Icon path={mdiAttachment} size={1} />
...

or

<Icon className='my-class' path={mdiMenu} />
<Icon className='my-class' path={mdiAccount} />
<Icon className='my-class' path={mdiAttachment} />
...

The latter having additional CSS just to get over the hurdle of making the icon appear. Both have more boilerplate than I think should be needed for an out-of-the-box and quick way to get an icon to appear in my app.


EDIT: Perhaps a default class goes on all icons of "mdi", so that at least all MDI icons can be targeted with CSS instead of needing to specify classes on each use.

So, with this proposal...

<Icon path={mdiMenu} />

Would render:

<svg viewbox="0 0 24 24" class="mdi">...</svg>

and

<Icon className='my-class' path={mdiMenu} />

Would render:

<svg viewbox="0 0 24 24" class="mdi my-class">...</svg>

I also like this approach, even though it does still require me to put something in my stylesheet just to get an icon to appear, which I find weird.

Templarian commented 5 years ago

Could definitely do that. It can't be mdi since that's what the webfont uses and I don't want to cause confusion. I could just add a default class of mdi-svg or something.

At work and with my various projects I tend to style them in their parent components.

button svg {
  width: 1.5rem;
  height: 1.5rem;
  path {
    fill: '#444';
  }
}
// Bootstrap
.btn.btn-sm { /* ... */ }

I'm fine adding a base class if it would make things easier. How is mdi-svg? I want to keep it less generic than icon to prevent any conflicts with frameworks.

Templarian commented 5 years ago

Prior to the v1.0 release of this it was defaulting to 1 for size, but was causing a lot of issues for people using it alongside their frameworks since it added the style attribute.

smeijer commented 5 years ago

Having this one resolved would be pretty nice 👍

alinnert commented 4 years ago

Another alternative would be to wrap the <Icon> in another component that sets the size to 1 by default. This can be done very easily by the library user.

But what if this library provides two components: one with a default size, one without? Like:

<Icon> and <SizedIcon>, or <BareIcon> and <Icon> (maybe someone finds better names)

Depending on what the size of <Icon> should be 1 or null. But leaving the default to null would at least be backwards compatible.

Templarian commented 4 years ago

Just an update on this. We'll probably never implement 1 as the default size in <Icon>. CSS should be used to configure the size and we don't want to force !important for setting size.

The @mdi/react component is also meant to be a bit more shared and 1 would imply 1.5rem or 24px icon size. This default may not be ideal for future icon packs maintained by MDI.

I'm by no means a React developer (actually haven't coded much React in the last year as I work primarily with web components), so any examples would be awesome to clarify why CSS might not be used to set the default icon size.

alinnert commented 4 years ago

@Templarian About your question: It's definitely possible (I think I prefer a wrapper component around the <Icon />). It's just not something you think of immediately. Also, the following thought process plays a big role here: "I'm using MDI. The MDI icon set uses a pixel grid of 24 x 24. Using a different size makes the icon look blurry. So, I shouldn't need to tell the icon how big it is, because it should know that already."

Now, one thing confuses me, though: Is @mdi/react a component to place MDI or any icon? From how the component can be used it feels very universal. In this context the missing default size makes perfect sense. But it's presented as a MDI only component where the missing default size (currently) doesn't make much sense. And I think this is part of the problem.

IMO it would be a good idea to change the name and/or the description and docs of @mdi/react and put an example in your docs on how to use it for a particular icon set.

Here's a basic example for a MDI React component:

import Icon from '@mdi/react'

export const MdiIcon = ({ path }) => (
  <Icon path={path} style={{ width: 24, height: 24 }} />
)

Then you can use it like this:

import { MdiIcon } from './MdiIcon.jsx'
import { mdiRefresh } from '@mdi/js'

/* ... */
  <MdiIcon path={mdiRefresh} />
/* ... */
AmirHosseinKarimi commented 4 years ago

Also can refer to #43 feature request to set a default class like mdi and also mdi-ICONNAME to modify the size and other property of all and specific icon by css class.