Skip to content
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

Standardize configuration / initialization / validation / messaging #166

Closed
JoeGermuska opened this issue Jul 10, 2015 · 2 comments · Fixed by #188
Closed

Standardize configuration / initialization / validation / messaging #166

JoeGermuska opened this issue Jul 10, 2015 · 2 comments · Fixed by #188

Comments

@JoeGermuska
Copy link
Member

At this time, problems creating a TimelineConfig option throw exceptions when there's no way to communicate the issue to the user. It is only when a Timeline instance exists that we have a mechanism for displaying error messages.

To improve messaging about error conditions, all cases where TimelineConfig throws exceptions should be adjusted to store those as error messages. Then, when a Timeline is initialized, the timeline should check for these messages as early as possible and use this.message.updateMessage to show the messages to whoever is at the browser.

It probably also makes sense to have an explicit validation step for the configuration. For example, in cases where there are no events in a spreadsheet (see #165), there is currently an exception thrown because of an invalid assumption deep inside Timeline. a TimelineConfig.validate() method could allow Timeline to abort processing and report error conditions before running into such a case.

Perhaps some of the things which are currently handled as thrown exceptions in the TimelineConfig initializer should be deferred to validation, although it may be harder to provide clear messaging if that's done.

By deferring exception throwing and moving error reporting into the Timeline constructor, we can use VCO.Language message keys to describe error conditions, so that initialization/setup problem messages could be localized, which might help Timeline creators who know enough English to create a Timeline but who would benefit from error reporting in their preferred language.

As a consequence of this, I think that TimelineConfig's ability to read JSON from a URL should be moved to VCO.ConfigFactory which is currently responsible for handling Google Spreadsheet URLs. Otherwise, calling validate on a TimelineConfig could be subject to timing problems because of the async AJAX call.

After these changes, Timeline instance could only be created with an instance of TimelineConfig, and a TimelineConfig instance would only be created by passing valid JSON into the constructor.

Alternatively, the TimelineConfig object could be hidden from the public API and then the signature would become new VCO.Timeline(elementId, json_obj, options) I'm not sure how I feel about this and welcome opinions.

@JoeGermuska JoeGermuska added this to the cutover milestone Jul 10, 2015
@JoeGermuska
Copy link
Member Author

related, the use of the VCO.Events mixin on VCO.TimelineConfig should be removed, because when those events fire right now there's nothing available to respond to them.

@JoeGermuska
Copy link
Member Author

This really is too tangled. I think it would be best to revise VCO.ConfigFactory so that it exposes a single public function, something like makeConfig(url,callback) The callback would be defined as a function which expects a populated VCO.TimelineConfig object as its single argument

This would let us squash the warning about using async, and it would provide a consistent API no matter what kind of URL is provided. It would also better fit the name VCO.ConfigFactory since right now it doesn't actually make configs.

JoeGermuska added a commit that referenced this issue Aug 1, 2015
…ine, so that we can make enough of a Timeline to have someplace to display the errors. #166
JoeGermuska added a commit that referenced this issue Aug 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant