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

feat: Add flexibility to get other props as a part of renderText function #484

Merged

Conversation

nikhil-varma
Copy link
Collaborator

@nikhil-varma nikhil-varma commented Jan 7, 2021

Describe the issue/change

In the current system, we cannot get access to the custom props that are passed in the renderText method.

Add CodeSandbox link to illustrate the issue (If applicable)

Describe the changes proposed/implemented in this PR

This PR introduces another parameter in the renderText method in order to get the props passed to the component

Link Github issue if this PR solved an existing issue

Issue: #456

Example usage (If applicable)

 <NumberFormat
        displayType="text"
        className="foo"
        someRandomProps="someValue"
        value="123"
        renderText={(formattedValue, props) => {
          // Do something with obtained props
          return <span>{formattedValue}</span>;
        }}
      />

Screenshot (If applicable)

Please check which browsers were used for testing

  • Chrome
  • Chrome (Android)
  • Safari (OSX)
  • Safari (iOS)
  • Firefox
  • Firefox (Android)

@s-yadav
Copy link
Owner

s-yadav commented Jan 11, 2021

@naman03malhotra @ParthS007, CI not working on PRs, can anyone take a look into this.

@ParthS007
Copy link
Contributor

I can make PR. @naman03malhotra Please let me know if you have already started working on this.

@ParthS007
Copy link
Contributor

@s-yadav I made the PR fixing the CI run. #486

After merging my PR, you have to change the required status for the CI with the new one.

@s-yadav
Copy link
Owner

s-yadav commented Jan 11, 2021

@ParthS007, Merged your changes. The action seems working correctly now.

@nikhil-varma nikhil-varma marked this pull request as ready for review January 11, 2021 17:59
@nikhil-varma
Copy link
Collaborator Author

@s-yadav Ready for review 😄

@ParthS007
Copy link
Contributor

@nikhil-varma Can you please rebase your branch with the latest master for CI to run. Thanks

@nikhil-varma
Copy link
Collaborator Author

@ParthS007 Done! 😄

@nikhil-varma
Copy link
Collaborator Author

nikhil-varma commented Jan 21, 2021

@s-yadav Can you please verify if the changes are good to go! 😄

@s-yadav
Copy link
Owner

s-yadav commented Jan 22, 2021

LGTM, can you add spec for the change.

@nikhil-varma
Copy link
Collaborator Author

@s-yadav Specs are added. Please review if it looks good! Thanks!

@s-yadav s-yadav merged commit 8d19e7e into s-yadav:master Mar 7, 2021
@nikhil-varma nikhil-varma deleted the feat/render-text-param-enhancement branch March 7, 2021 18:29
@nikhil-varma
Copy link
Collaborator Author

nikhil-varma commented Mar 7, 2021

@s-yadav Should we go ahead and close the linked issue? Or wait until we release the new patch/version and then do it? What is the convention that we follow?

@nikhil-varma nikhil-varma changed the title feat: Add flexibility to get other props as a part of render function feat: Add flexibility to get other props as a part of renderText function Mar 7, 2021
@s-yadav
Copy link
Owner

s-yadav commented Mar 7, 2021

Will release a new version.
One thing we missed, we will have to update the readme to reflect this new param.

@nikhil-varma
Copy link
Collaborator Author

nikhil-varma commented Mar 7, 2021

@s-yadav Right! I will update the readme and create a PR for this.

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.

4 participants