Skip to content

Add NewStyled method for creating a globe with a custom style - #7

Open
taywrobel wants to merge 2 commits into
mmcloughlin:masterfrom
taywrobel:master
Open

Add NewStyled method for creating a globe with a custom style#7
taywrobel wants to merge 2 commits into
mmcloughlin:masterfrom
taywrobel:master

Conversation

@taywrobel

@taywrobel taywrobel commented Aug 20, 2017

Copy link
Copy Markdown

Introduces a new constructor function "NewStyled", which takes as a
parameter a style and constructs an empty globe with that style.

This gives control over controls in the style object which aren't
accessible by default or by using Options, and without changing
the DefaultStyle, which is globally scoped and may have unintended
side effects if changed.

Introcues a new constructor function "NewStyled", which takes as a
parameter a style and constructs an empty globe with that style.

This gives control over controls in the style object which aren't
accessible by default or by using Options.
@mmcloughlin

Copy link
Copy Markdown
Owner

@twrobel3 thanks for the pull request!

I wonder about using the functional options pattern. In this case you could change New() to New(options ...Option) and add a WithStyle option.

It's a bit late now but I wish I had distinguished between "style" options and constructor options. Color is also an Option but wouldn't make much sense to pass to New(). Any thoughts on this?

@taywrobel

Copy link
Copy Markdown
Author

I agree that "style" options are semantically different than some of the other options used.

The inspiration here was that I didn't see any good way way to change the background color of the rendering without changing the DefaultStyle struct. The Image(side int) signature could be updated with the styles option as well, passing in a color there, and achieve the same effect, but the same issue would come in with some other struct members, like LineWidth, which doesn't appear to be something that can be treated as an Option, unless I'm missing something.

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.

2 participants