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

Replace "firstRun" config option with "openSidebar" and "openLoginForm" #63

Merged
merged 2 commits into from
Aug 3, 2016

Conversation

nickstenning
Copy link
Contributor

This commit deprecates the confusingly-named "firstRun" embed script option with two which actually describe the relevant actions triggered.

Unfortunately, at the moment this requires a fairly horrific check that the string value of a config option is not the literal 'false', due to the way that the Host code passes configuration from the page frame to the sidebar frame (using URL query string parameters).

To make up for that, I've added a docs/config.md file in which to document these configuration options.

This commit deprecates the confusingly-named "firstRun" embed script
option with two which actually describe the relevant actions triggered.

Unfortunately, at the moment this requires a fairly horrific check that
the string value of a config option is not the literal 'false', due to
the way that the Host code passes configuration from the page frame to
the sidebar frame (using URL query string parameters).
@nickstenning
Copy link
Contributor Author

See also hypothesis/h#3643, which should be merged and deployed before this.

@nickstenning
Copy link
Contributor Author

We should probably also actually detect the firstRun option in use (for embeds, for example) and emit a console deprecation warning.

@seanh
Copy link
Contributor

seanh commented Aug 3, 2016

LGTM. @chdorner any reason not to merge this now?

@chdorner
Copy link
Contributor

chdorner commented Aug 3, 2016

@seanh should be good to go now, I just released the corresponding change in h to production (which sets old and new configuration settings)

@robertknight
Copy link
Member

Unfortunately, at the moment this requires a fairly horrific check that the string value of a config
option is not the literal 'false', due to the way that the Host code passes configuration from the page
frame to the sidebar frame (using URL query string parameters).

Possible dumb but simple solution:

host.coffee:
  options.app += '?config=' + btoa(JSON.stringify(appOpts))

app.js:
  settings.config = JSON.parse(atob(queryString.parse(window.location.search).config));
  // change references to settings.<configOpt> to settings.config.<configOpt>

@robertknight
Copy link
Member

LGTM. I'll merge this and post a potential simplification as a follow-up

@robertknight robertknight merged commit 632846d into master Aug 3, 2016
@robertknight robertknight deleted the remove-firstrun-config-option branch August 3, 2016 13:57
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.

4 participants