-
Notifications
You must be signed in to change notification settings - Fork 16
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: expose createTheme as createCartoTheme #138
Conversation
Pull Request Test Coverage Report for Build 729750896
💛 - Coveralls |
@@ -1175,16 +1175,10 @@ export const cartoThemeOptions = { | |||
} | |||
}; | |||
|
|||
export function createTheme(options = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we can remove default options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As described in documentation
PR, the way to overwrite cartoThemeOptions
is equal for both implementations in this way.
Also, passing to createTheme
and object like:
const mainColor = { palette: { primary: { main: '#80000' }}}
results in an error. Do you have any other idea to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we leave the optional param, and just expose the method?
That way we could also allow overriding certain properties, so we would eventually get 3 options for the user:
- call to createTheme without params (one line from the client)
- call to createTheme with certain options (maybe just colors)
- completely custom case, using just cartoThemeOptions (what is currently being used from template)
/cc @borja-munoz
Please add a note to changelog and link here the PR with the doc change |
We should talk more about this one with @borja-munoz, before exposing it |
Not a clear advantage on the change, so I'm closing this |
Expose
cartoTheme
ascreateCartoTheme
.Related with
documentation
: CartoDB/documentation#78