slamdata / purescript-echarts

Purescript bindings for Baidu's Echarts library
36 stars 16 forks source link

Add theme support #30

Closed safareli closed 7 years ago

safareli commented 7 years ago
cryogenian commented 7 years ago

Oh! Yep! Let's define an ADT

data BuiltInTheme = Infographic | Macarons | Roma | Shine | Vintage | Dark 
Then define a func from it to string like

builtInThemeRoute :: BuiltInTheme -> String 
builtInThemeRoute = case _ of 
  Roma -> "echarts/theme/roma"
  _ -> ... 

builtinTheme :: BuiltInTheme -> Theme 
builtinTheme = case _ of 
  Roma -> "roma"
  _ -> ...

Then define a func that registers builtintheme

foreign import registerBuiltin_ ::  String-> Eff _ Unit
registerBuiltin :: BuiltInTheme -> Eff _ Theme
registerBuiltIn bit = builtinTheme bit <$ registerBuiltin_ (builtinThemeRoute bit)

and in Theme.js

exports.registerBuiltIn_ = function(route) {
  return function() {
    require(route);
  };
};

This way it's cleaner that registering builtin themes are effectful and they're actually predefined. And preserves notion of themes being open to extension by outside code.

safareli commented 7 years ago

@cryogenian the option looks nice!

But it might have issues with dead code elimination. I think code analyzers will not be able to determine required pathes which are dynamic, if that issue is not a problem Will implement it as you pointed out.

natefaubion commented 7 years ago

I would recommend not relying on dynamic requires.

cryogenian commented 7 years ago

@natefaubion In this case it's actually isn't require but just an effect that registers an object as theme. (Basically stores an object to global map). So, yeah I know that this will bundle all themes to output, I'm not sure that this is a problem though.

natefaubion commented 7 years ago

The bundler can't pick up opaque dynamic requires though.

safareli commented 7 years ago

I think it would be better to just state in readme to include themes client is want's to use in there bundler, as otherwise we have two options:

Do you agree, or am I missing something?

cryogenian commented 7 years ago

Ok, I think this is good to go. BTW, we can also use one module per theme. This way if user import theme it's required. But I think this is a matter of different pr.