Skip to content

Fix string arguments in component tree to render only one pair of double quotes fixes #1190#1193

Merged
chancancode merged 5 commits intoemberjs:masterfrom
SYU15:fix-string-args
May 12, 2020
Merged

Fix string arguments in component tree to render only one pair of double quotes fixes #1190#1193
chancancode merged 5 commits intoemberjs:masterfrom
SYU15:fix-string-args

Conversation

@SYU15
Copy link
Copy Markdown
Member

@SYU15 SYU15 commented May 8, 2020

This was a bad merge while experimenting with refactoring the surrounding string code for argument values.

Reverting the logic back to the way it was here: https://github.com/emberjs/ember-inspector/pull/1175/files#diff-a9290a87bac9f17e7512cb19a835f822L14 (in app/helpers/component-argument.js)

After fix

Screen Shot 2020-05-07 at 4 01 19 PM

@chancancode
Copy link
Copy Markdown
Member

Unfortunately we don't have a good way to test this right now, the integration test harness is broken for some reason, but I'm looking into it.

@RobbieTheWagner
Copy link
Copy Markdown
Member

I forget why we needed this stuff in the first place. Were we trying to handle if the strings included " in their output I think? I think the idea was make sure we remove all of them, then add some surrounding ourselves.

@chancancode
Copy link
Copy Markdown
Member

@rwwagner90 Yeah, the surrounding quotes is added in the template outside, so that the quotes are not part of the highlighting. This point is mostly moot now that we refactored it from being a helper to the component, and the component has its own template. This is the refactor that I was referring to. The quotes should still be added in template-land (instead of inside this JavaScript function), just that it should be moved from the tree-item component template into the tree-arg component template.

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented May 12, 2020

Unfortunately we don't have a good way to test this right now, the integration test harness is broken for some reason

Fixed by #1203.

@SYU15 - If you rebase against master, you should be able to use standard setupRenderingTest style tests here.

@chancancode chancancode merged commit 4b3d8ab into emberjs:master May 12, 2020
@SYU15
Copy link
Copy Markdown
Member Author

SYU15 commented May 12, 2020

Thanks @rwjblue @chancancode @rwwagner90!

patricklx pushed a commit to patricklx/ember-inspector that referenced this pull request Sep 19, 2022
…ble quotes fixes emberjs#1190 (emberjs#1193)

* Fix string arguments in component tree to render only one pair of double quotes
* Add component integration test for component-tree-arg
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.

4 participants