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

Unscrambling widget configuration #14

Open
karger opened this issue Jul 23, 2013 · 0 comments
Open

Unscrambling widget configuration #14

karger opened this issue Jul 23, 2013 · 0 comments

Comments

@karger
Copy link
Member

karger commented Jul 23, 2013

Exhibit is generally used to create widgets that are configured based on attributes in dom elements describing the widgets. However, the codebase is cluttered with alternative code that configures an element based on a passed-in "configuration" js object. I don't think this code has been used for years, if ever, and I suspect it will break immediately if anyone tries to use it.

As one concrete example, util/settings.js implements a "_internalCollectSettings" method (used for configuring from dom and js object) that reads values from the configuration and uses them to determine settings for the widget. When you read values from a dom object, you'll always get strings. But if you read from a configuration object you might get other types. internalCollectSettings seems to be trying to prepare for this, since to decide whether to process a value it tests for (typeof value === "string" && value.length ? 0) || (typeof value !== "string") . However, once the test is passed and internalCollectSettings begins to process the value, it invokes "value.split(;)" which is going to throw an error on anything but a string, which means non-string values won't be incorporated in the settings.

More generally, there are lots of inconsistent code pathways based on whether you invoke configure() or configureFromDom() and I'm pretty sure the inconsistency means the configure() methods just won't work right. For example, in list-facet.js there is significant code duplication between configureFromDom() and configure(), even though configureFromDom calls configure.

I'd like to clean up this clutter. The most aggressive way to do this would be simply to remove all the functions that configure from a js object instead of a dom node. If anyone is using these functions, please speak up! A somewhat less aggressive approach would be to better merge the configure() and configureFromDom() methods so that configure is less reliant on never-used code. However I still won't be testing the non-dom pathways, so there's no guarantee they'll work.

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

No branches or pull requests

1 participant