Code cleanup - Githubissues

mmazzarolo commented 7 years ago

Hi there, thanks for the library.
I noticed that the code of the lib can be simplified/cleaned up a lot, if you're interested here is a solution similar to the one I adopted in my current project (I used Flowtype in my project, so I stripped it from this example).
Let me know if you're interested in a PR.

// ./index.js
import React, { Component, PropTypes } from 'react';
import { StyleSheet, WebView, Dimensions } from 'react-native';
import { flattenObject } from './utils';
import html from './page.html';

const DEVICE_HEIGHT = Dimensions.get('window').height;
const DEVICE_WIDTH = Dimensions.get('window').width;

export default class ReactNativeHighcharts extends Component {
  static propTypes = {
    configuration: PropTypes.object.isRequired,
    style: PropTypes.any,
  };

  state = {
    visible: true,
    height: DEVICE_HEIGHT,
    width: DEVICE_WIDTH,
  };

  // Used to resize on orientation of display
  _reRenderWebView = e => {
    this.setState({
      height: e.nativeEvent.layout.height,
      width: e.nativeEvent.layout.width,
    });
  };

  // Created the javascript that will be injected in the webview
  _createInjectedJavascript = configuration => {
    const stringConfiguration = JSON.stringify(configuration, (key, value) => {
      return typeof value === 'function' ? value.toString() : value;
    });
    const parsedConfiguration = JSON.parse(stringConfiguration);
    const injectedJavascript = `
      var chart = Highcharts.chart('container', ${flattenObject(parsedConfiguration)});
      chart.reflow();
      setTimeout(function() {chart.reflow()}, 0);
      setTimeout(function() {chart.reflow()}, 100);
    `;
    return injectedJavascript;
  };

  render() {
    const { configuration, style, ...otherProps } = this.props;
    return (
      <WebView
        onLayout={this._reRenderWebView}
        style={[styles.webView, style]}
        source={html}
        javaScriptEnabled={true}
        domStorageEnabled={true}
        scalesPageToFit={false}
        scrollEnabled={false}
        automaticallyAdjustContentInsets={false}
        injectedJavaScript={this._createInjectedJavascript(configuration)}
        {...otherProps}
      />
    );
  }
}

const styles = StyleSheet.create({
  webView: {
    flex: 1,
    backgroundColor: 'transparent',
  },
});

// ./utils.js
export const flattenObject = (obj: Object, str: string = '{') => {
  Object.keys(obj).forEach(function(key) {
    str += `${key}: ${flattenText(obj[key])}, `;
  });
  return `${str.slice(0, str.length - 2)}}`;
};

const flattenText = (item: any) => {
  var str = '';
  if (typeof item === 'object' && item.length == undefined) {
    str += flattenObject(item);
  } else if (typeof item === 'object' && item.length !== undefined) {
    str += '[';
    item.forEach(function(k2) {
      str += `${flattenText(k2)}, `;
    });
    str = str.slice(0, str.length - 2);
    str += ']';
  } else if (typeof item === 'string' && item.slice(0, 8) === 'function') {
    str += `${item}`;
  } else if (typeof item === 'string') {
    str += `\"${item.replace(/"/g, '\\"')}\"`;
  } else {
    str += `${item}`;
  }
  return str;
};

export default {
  flattenObject,
};

<!-- ./html -->
<!DOCTYPE html>
<html>
  <head>
    <meta name="viewport" content="width=device-width,initial-scale=1,user-scalable=no">
    <style media="screen" type="text/css">
      html, body {
        margin: 0;
        padding: 0;
        width: 100%;
        height: 100%;
      }
      #container {
        width:100%;
        height:100%;
        top:0;
        left:0;
        right:0;
        bottom:0;
        position:absolute;
      }
    </style>
  </head>
  <body>
    <div id="container"></div>
    <script src="https://code.highcharts.com/highcharts.js"></script>
    <script src="https://code.highcharts.com/modules/exporting.js"></script>
  </body>
</html>

Infinity0106 commented 7 years ago

Don't you think the code is too short to create multiple files?, I didn't know that you could import html

mmazzarolo commented 7 years ago

I don't think so, especially if it is a library which might grow in the future (but it also is a matter of personal preference, of course :) ) The code linting and styiling though should be required for PRs in my opinion, so that you can avoid issues like #14 but whatever.
Just my 2 cents, I opened an issue instead of a PR because I'm aware that it's also a matter of taste!

jonrh commented 7 years ago

I agree that this project is now at a stage where we should maybe start thinking about an agreed upon linting procedure. I hear prettier 💅 is all the rage these days 💪. Should we perhaps create a separate issue for it?

mmazzarolo commented 7 years ago

I can't praise Prettier higly enough, it simply rocks!

TradingPal / react-native-highcharts

Code cleanup #15