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 support for setting an onError prop on SvgCssUri #1718

Merged
merged 5 commits into from
Apr 12, 2022

Conversation

SjaakSchilperoort
Copy link
Contributor

Summary

In PR #1266, support was added for setting an onError property on SvgUri. This PR adds the same to SvgCssUri.

Test Plan

This should now work:

import React, {Component} from 'react';
import {  SvgCssUri } from 'react-native-svg';

class Example extends Component {

    state={
      uri: 'https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/xruby.svg',
    }

    render() {
        return (
            <SvgCssUri width={100} onError={() => this.setState({uri: 'https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/ruby.svg'})} height={100} uri={this.state.uri} />
        );
    }
}

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

OS Implemented
iOS ✅❌
Android ✅❌

Checklist

  • I have tested this on a device and a simulator
  • I added documentation in README.md
  • I updated the typed files (typescript)
  • I added a test for the API in the __tests__ folder

Note

This PR is on request from @WoLewicki in PR #1503.

src/index.d.ts Outdated Show resolved Hide resolved
@WoLewicki
Copy link
Member

Also, could you add a test example in TestsExample app with those simple code snippets?

@WoLewicki
Copy link
Member

Is it ready to be merged in your opinion @SjaakSchilperoort ?

@SjaakSchilperoort
Copy link
Contributor Author

I looked into adding example code in TestsExample, but I think it makes more sense to document the use of onError in README.md. If you agree I can add an example there, but otherwise I'm fine with this change.

@WoLewicki
Copy link
Member

If it is possible, I would opt for including both documentation and a test in TestsExample.

@SjaakSchilperoort
Copy link
Contributor Author

If it is possible, I would opt for including both documentation and a test in TestsExample.

Done

Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉 Thanks for your contribution!

@WoLewicki WoLewicki merged commit 69e1be7 into software-mansion:main Apr 12, 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.

2 participants