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

Use double quotes for embed iframes #4592

Merged
merged 1 commit into from
Jan 13, 2023
Merged

Use double quotes for embed iframes #4592

merged 1 commit into from
Jan 13, 2023

Conversation

jeremy-rifkin
Copy link
Member

This fixes the core issue with #4541 and closes #4541, separately I'll add better diagnostics for the user for when things go wrong

@github-actions github-actions bot added the ui label Jan 13, 2023
@mattgodbolt
Copy link
Member

Reading the rison spec it's a miracle this ever worked! ("single-quotes are used for quoting, but quotes can and should be left off strings when the strings are simple identifiers.") so rison itself would use ' ... though maybe we also need to URI encode? :| what if the embedded thing has any time of quote in it?

@jeremy-rifkin jeremy-rifkin merged commit 3d144f6 into main Jan 13, 2023
@jeremy-rifkin jeremy-rifkin deleted the jr/fix/4541 branch January 13, 2023 21:16
@jeremy-rifkin
Copy link
Member Author

Our full links seem to work without URI encoding the whole thing and while rison uses ' I don't think it ever spits out "

@jeremy-rifkin
Copy link
Member Author

Ah shoot actually it can output a " however it looks like the source part of our states is URI encoded so we don't have to worry about double quotes in source. Not sure if it's rison or something else doing that URI encoding.

@partouf
Copy link
Contributor

partouf commented Jan 13, 2023

Ah shoot actually it can output a " however it looks like the source part of our states is URI encoded so we don't have to worry about double quotes in source. Not sure if it's rison or something else doing that URI encoding.

probably LZString?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: Embedded mode is broken
3 participants