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

Fix secondaryFill and secondaryOpacity errors by renaming to lowercase #74

Closed
wants to merge 2 commits into from

Conversation

rvdende
Copy link

@rvdende rvdende commented Oct 1, 2020

I was getting a react errors when using this in react-native-web:

Warning: React does not recognize the `secondaryFill` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `secondaryfill` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
    in path (created by Path)
    in Path (created by FontAwesomeIcon)
    in svg (created by Svg)
    in Svg (created by FontAwesomeIcon)
    in FontAwesomeIcon (created by NavButton)
Warning: React does not recognize the `secondaryOpacity` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `secondaryopacity` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
    in path (created by Path)
    in Path (created by FontAwesomeIcon)
    in svg (created by Svg)
    in Svg (created by FontAwesomeIcon)
    in FontAwesomeIcon (created by NavButton)

I resolved the issue by renaming these as recommended. Tests still pass so I think it's fine?

@joker-777
Copy link

Hi, I get the same error.

@workmaxtim
Copy link

When is are the commits 615577d and 8768e9d going to be in the release?

@rvdende
Copy link
Author

rvdende commented Nov 3, 2020

My guess is this could potentially break the layout on mobile apps because of the prop name change... for now I just npm install direct from my repo :)

@ChanceLeachman
Copy link

Any update on this?

@robmadole
Copy link
Member

I don't think that secondaryfill or secondaryopacity should be applied to any DOM elements. These properties are both used in the converter (the thing that takes the abstract Font Awesome icon and renders it using 'react-native-svg') and has no value beyond calculating the fill and fillOpacity values.

A better fix for this would be to remove the secondaryFill and secondaryOpacity from the extraProps before it's passed to the Path element.

That should happen around here.

@robmadole robmadole mentioned this pull request May 11, 2021
@micheleb
Copy link

Hey there 👋🏻
Confirming that what @robmadole mentioned works, thanks! 🎉

Here's the patch I created using patch-package which made the error go away, in case it's helpful:

patches/@FortAwesome+react-native-fontawesome+0.2.6.patch

diff --git a/node_modules/@fortawesome/react-native-fontawesome/dist/converter.js b/node_modules/@fortawesome/react-native-fontawesome/dist/converter.js
index 5b737a8..3849cd6 100644
--- a/node_modules/@fortawesome/react-native-fontawesome/dist/converter.js
+++ b/node_modules/@fortawesome/react-native-fontawesome/dist/converter.js
@@ -41,6 +41,9 @@ var svgObjectMap = {
 
 function convert(createElement, element) {
   var extraProps = arguments.length > 2 && arguments[2] !== undefined ? arguments[2] : {};
+  var withoutFillProps = _objectSpread({}, extraProps);
+  delete withoutFillProps.secondaryFill;
+  delete withoutFillProps.secondaryOpacity;
 
   if (typeof element === 'string') {
     return element;
@@ -51,7 +54,7 @@ function convert(createElement, element) {
     var isDuotoneSecondLayer = isDuotone && childIndex === 0;
     var fill = isDuotoneSecondLayer ? extraProps.secondaryFill : extraProps.fill;
     var fillOpacity = isDuotoneSecondLayer ? extraProps.secondaryOpacity : 1;
-    return convert(createElement, child, _objectSpread(_objectSpread({}, extraProps), {}, {
+    return convert(createElement, child, _objectSpread(withoutFillProps, {}, {
       fill: fill,
       fillOpacity: fillOpacity
     }));
@@ -84,7 +87,7 @@ function convert(createElement, element) {
   }, {
     attrs: {}
   });
-  return createElement.apply(void 0, [svgObjectMap[element.tag], _objectSpread(_objectSpread({}, mixins.attrs), extraProps)].concat(_toConsumableArray(children)));
+  return createElement.apply(void 0, [svgObjectMap[element.tag], _objectSpread(_objectSpread({}, mixins.attrs), withoutFillProps)].concat(_toConsumableArray(children)));
 }
 
 var _default = convert;

This is based on the dist folder, so it's using _objectSpread etc. I'll submit a PR as soon as I have a bit more time (provided that this one doesn't get updated/merged in the meantime), maybe next weekend.

@ivanee
Copy link

ivanee commented Aug 20, 2021

@micheleb: remove the secondaryFill and secondaryOpacity only from the final return value of converter(). They are required in the recursive call.

This is my patch file:
patches/@FortAwesome+react-native-fontawesome+0.2.6.patch:

diff --git a/node_modules/@fortawesome/react-native-fontawesome/dist/converter.js b/node_modules/@fortawesome/react-native-fontawesome/dist/converter.js
index 5b737a8..e3f220f 100644
--- a/node_modules/@fortawesome/react-native-fontawesome/dist/converter.js
+++ b/node_modules/@fortawesome/react-native-fontawesome/dist/converter.js
@@ -84,8 +84,11 @@ function convert(createElement, element) {
   }, {
     attrs: {}
   });
-  return createElement.apply(void 0, [svgObjectMap[element.tag], _objectSpread(_objectSpread({}, mixins.attrs), extraProps)].concat(_toConsumableArray(children)));
-}
+
+  var withoutFillProps = _objectSpread({}, extraProps);
+  delete withoutFillProps.secondaryFill;
+  delete withoutFillProps.secondaryOpacity;
+  return createElement.apply(void 0, [svgObjectMap[element.tag], _objectSpread(_objectSpread({}, mixins.attrs), withoutFillProps)].concat(_toConsumableArray(children)));}
 
 var _default = convert;
 exports["default"] = _default;
\ No newline at end of file

@WillyRamirez
Copy link

Any idea when this gets merged? And Is there a workaround we can follow until then?

@Muffinoota
Copy link

Any idea when this gets merged? And Is there a workaround we can follow until then?

Consider using @fortawesome/react-fontawesome on web.
You can use react native's platform module to determine if the application is built for web and use the react module in those cases.

import {FontAwesomeIcon as FontAwesomeReact} from '@fortawesome/react-fontawesome';
import {FontAwesomeIcon as FontAwesomeNative} from '@fortawesome/react-native-fontawesome';
import {number} from 'prop-types';
import React from 'react';
import {Platform, StyleSheet} from 'react-native';

FaIcon.propTypes = {
  size: number,
};
export default function FaIcon({size, style, ...props}) {
  if (Platform.OS === 'web') {
    const webStyles = StyleSheet.flatten([style, {width: size, height: size}]);
    return <FontAwesomeReact {...props} style={webStyles} />;
  }

  return <FontAwesomeNative {...props} size={size} style={style} />;
}

@charlestbell
Copy link

Thanks @Muffinoota

I'm building an expo app, and my implementation looks like this:

// New file called font-awesome-utils.js
import { FontAwesomeIcon as nativeFontAwesomeIcon } from '@fortawesome/react-native-fontawesome';
import { FontAwesomeIcon as reactFontAwesomeIcon } from '@fortawesome/react-fontawesome';
import { Platform } from 'react-native';

export * from '@fortawesome/react-native-fontawesome';

const FontAwesomeIcon =
  Platform.OS === 'web' ? reactFontAwesomeIcon : nativeFontAwesomeIcon;

export { FontAwesomeIcon };
// In my component's file
import { FontAwesomeIcon } from '../../utils/font-awesome-utils';

Not sure if it's the best way, but it removed the console errors on web for me.

@robmadole
Copy link
Member

Ok, we've finally spent some time on this and it was crazier than we first thought. Our PR #134 has a proper fix for this. We'll get this released pretty soon.

@robmadole
Copy link
Member

Ok release 0.3.0 should have this all fixed up! Let me know if that's not the case and I'll re-open.

@robmadole robmadole closed this Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants