8000 first needed modifications to support react by tobiaslins · Pull Request #25 · frappe/charts · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

first needed modifications to support react #25

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 4, 2017
Merged

first needed modifications to support react #25

merged 6 commits into from
Nov 4, 2017

Conversation

tobiaslins
Copy link
Contributor
Explanation About What Code Achieves:
  • It will support a html component as a parent which is needed for react.
  • It will store the generated css into a file that it can be used within react
Steps To Test:
  • Use branch and create "basic" react component as described in Support React #24.
  • Import and use this component and check if the chart is rendered corretly
TODOs:
  • We should discuss if we want to support React per default -> package.json main should point on a non rollupjs file

achillesrasquinha
achillesrasquinha reviewed Nov 3, 2017
View reviewed changes
Copy link
Contributor
@achillesrasquinha achillesrasquinha left a comment

Choose a reason for hiding this comment

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

Add yarn.lock to .gitignore

pratu16x7
pratu16x7 reviewed Nov 4, 2017
View reviewed changes
Copy link
Contributor
@pratu16x7 pratu16x7 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@tobiaslins So we've gone ahead and kept the commonjs module in main, in addition to adding in the es6 module and the iife for browser use (ref #35)
Apart from that, we'd be happy to merge once rebased :)

@tobiaslins
Copy link
Contributor Author

@pratu16x7 thank you! :)
Yes makes sense, should be ok now?

@pratu16x7 pratu16x7 merged commit 3a52a56 into frappe:master Nov 4, 2017
@pratu16x7 pratu16x7 mentioned this pull request Nov 5, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pratu16x7 pratu16x7 pratu16x7 left review comments

@achillesrasquinha achillesrasquinha achillesrasquinha left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tobiaslins @pratu16x7 @achillesrasquinha
0