FortAwesome / vue-fontawesome

Font Awesome Vue component
https://fontawesome.com
MIT License
2.38k stars 134 forks source link

Vue component considers 0 (for rotation) and md (for size) despite them being legitimate values #493

Open mattburlage opened 3 months ago

mattburlage commented 3 months ago

The vue component throws a warning when rotation is 0 or size is md, making it difficult to determine those values programmatically. Having those as defaults is reasonable, but the component should also accept those values explicitly.

<FontAwesomeIcon /> shouldn't consider 0 (for rotation) or 'md' (for size) as invalid prop values.

For example:

<FontAwesomeIcon
   :icon="ICON"
   :rotation="ROTATION"
   :size="SIZE"
/>
Emmo00 commented 3 months ago

This rotation props controls the fa-rotate- class, and according to the (docs)[https://docs.fontawesome.com/web/style/rotate], the only angles available are: 90, 180 and 270.

a temporary fix (to remove the warning) might be to just add 0 to the list of expected values by the vue component, this would add a fa-rotate-0 class to the icon - which does not have a style implemented.

Another way might be to implement (Custom fix)[https://docs.fontawesome.com/web/style/rotate#custom-rotation] for the rotation prop... this might be a more involved patch.

I'd be happy to write a patch

mattburlage commented 3 months ago

Would it be possible for the component to just not add fa-rotate- if it detects 0? Likewise, not apply a size if md is detected for that attribute? Maybe here?

https://github.com/FortAwesome/vue-fontawesome/blob/95557e3aaa9d8dd90766fdde8d4df9a6bf0d8d4f/src/utils.js#L5

Something like:

export function classList (props) {
  const size = props.size !== 'md' ? props.size : null;
  const rotation = props.rotation !== 0 ? props.rotation : null;

  let classes = {
    'fa-spin': props.spin,
    'fa-pulse': props.pulse,
    'fa-fw': props.fixedWidth,
    'fa-border': props.border,
    'fa-li': props.listItem,
    'fa-inverse': props.inverse,
    'fa-flip': props.flip === true,
    'fa-flip-horizontal': props.flip === 'horizontal' || props.flip === 'both',
    'fa-flip-vertical': props.flip === 'vertical' || props.flip === 'both',
    [`fa-${size}`]: size !== null,
    [`fa-rotate-${rotation}`]: rotation !== null,
    [`fa-pull-${props.pull}`]: props.pull !== null,
    'fa-swap-opacity': props.swapOpacity,
    'fa-bounce': props.bounce,
    'fa-shake': props.shake,
    'fa-beat': props.beat,
    'fa-fade': props.fade,
    'fa-beat-fade': props.beatFade,
    'fa-flash': props.flash,
    'fa-spin-pulse': props.spinPulse,
    'fa-spin-reverse': props.spinReverse
  }

  return Object.keys(classes)
    .map(key => classes[key] ? key : null)
    .filter(key => key)
}

Since the object definition already handles if those attributes are null, this should accomplish the goal without adding extra classes. Thoughts?

mattburlage commented 3 months ago

I suppose you could also just handle this in the object key-value assignments and avoid the extra variables.

    [`fa-${props.size}`]: ['md', null].includes(props.size),
    [`fa-rotate-${props.rotation}`]: [0, null].includes(props.rotation),
Emmo00 commented 3 months ago

You're right

Emmo00 commented 3 months ago
    [`fa-${props.size}`]: ['md', null].includes(props.size),
    [`fa-rotate-${props.rotation}`]: [0, null].includes(props.rotation),

Yes, this looks nice, but Did you mean to negate this like

[`fa-rotate-${props.rotation}`]: ![0, null].includes(props.rotation)

?

mattburlage commented 3 months ago

Yes, good catch.