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

fix(sdk): set the hideNavigation param correctly for embeds #1654

Merged
merged 2 commits into from
Nov 5, 2021

Conversation

fvsch
Copy link
Contributor

@fvsch fvsch commented Nov 5, 2021

We have a bug with the hideNavigation option for embed methods.

Instead of adding hideNavigation=1 to the URL, we add hideNavigation=1;, resulting in a value of 1; which gets ignored by the StackBlitz editor.

This PR fixes this issue, and constructs the query string by joining an array of key=value strings instead of adding having to deal with adding & or not for each parameter.

There are a couple other changes:

  1. The theme parameter is restricted to 'light' or 'dark'. Though maybe we should accept any string, even though we're not supporting other values? Happy to roll back that change if we want to keep it open. (I’m noticing that it's not documented on https://developer.stackblitz.com/docs/platform/javascript-sdk/#embed-options yet?)

  2. The devtoolsheight option now accepts the values 0 and 100. In particular, I’ve seen users of the SDK use devToolsHeight: 99 because 100 was filtered out by the SDK, even though devtoolsheight=100 in the URL does work.

@fvsch fvsch requested a review from sulco November 5, 2021 16:06
@sulco sulco merged commit 9da2c08 into master Nov 5, 2021
@sulco sulco deleted the fix/sdk-hidenavigation-param branch November 5, 2021 16:12
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