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

Add tests #23

Open
pke opened this issue May 21, 2020 · 2 comments
Open

Add tests #23

pke opened this issue May 21, 2020 · 2 comments

Comments

@pke
Copy link

pke commented May 21, 2020

This project lacks some tests to verify the functionality.
Do you have any plans to add tests or would accept a PR?

Case in point:

export const setCustomText = customProps => {
  const TextRender = Text.render
  const initialDefaultProps = Text.defaultProps
  Text.defaultProps = {
    ...initialDefaultProps,
    ...customProps
  }
  Text.render = function render(props) {
    let oldProps = props
    props = { ...props, style: [customProps.style, props.style] }
    try {
      return TextRender.apply(this, arguments)
    } finally {
      props = oldProps
    }
  }
}

This code can not work. It calls TextRender.apply with the original arguments, it assigns new props ignoring the Text.defaultProps.style and then assigns the props back in the finally block to no effect.

Tests would discover this faulty behaviour.

A working version of the above code seems to be:

const setCustomText = (customProps) => {
  Text.defaultProps = {
    ...Text.defaultProps,
    ...customProps,
  }
  const orgRender = Text.render
  Text.render = function render(props:TextProps, ref:Ref<Text>) {
    const style = Array.isArray(props.style) ? props.style : [props.style]
    props = {
      ...props,
      style: [Text.defaultProps.style, ...style],
    }
    return orgRender.call(this, props, ref)
  }
}
@Ajackster
Copy link
Owner

Thanks for creating this issue. I would definitely accept a PR with added tests.

@pke
Copy link
Author

pke commented May 22, 2020

Thanks for your feedback! I wonder how could the current code ever work? Or was just nobody using it?

I also wonder why RN does not offer this by default. I tried to search the issues over there but no-one seems to be missing setting a font globally for all Text elements. Everyone seems to be happy to derive a custom Text component for their apps that no one developer of this app should ever forget to use. And then there are 3rd party components that render just the default RN Text component and sometimes don't give you the styling options to change that.

Really confusing why this is not part of the default RN lib.

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

No branches or pull requests

2 participants