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

Adjust component tree colors #1109

Closed
wants to merge 7 commits into from
Closed

Adjust component tree colors #1109

wants to merge 7 commits into from

Conversation

nummi
Copy link
Collaborator

@nummi nummi commented Dec 18, 2019

@rwwagner90, here's a PR for your PR that is a PR for another PR.

image

image

@chancancode
Copy link
Member

Having seen both, I think I actually like showing the @ sign as the same color as the identifier a lot more! IMO, just like instance variables in Ruby, the @ sign is essentially part of the variable name (since @foo and foo are talking about two different variables), the same color really helps drive that point home.

@nummi
Copy link
Collaborator Author

nummi commented Dec 18, 2019

@chancancode, @rwwagner90, can you take a look at the change I made here: https://github.com/emberjs/ember-inspector/pull/1109/files#diff-a9290a87bac9f17e7512cb19a835f822R12 ?

It looks like the attr value coming out of Debug has double quotes around it — the result is the quotes being highlighted with the actual value. I felt it makes more sense for the quotes to be grey like the other less important syntax bits:

image

@chancancode
Copy link
Member

@nummi seems good to me

@@ -9,7 +9,7 @@ import { helper } from '@ember/component/helper';
*/
export function componentArgumentDisplay([argument]) {
if (typeof argument === 'string') {
return JSON.stringify(argument);
return JSON.stringify(argument).replace(/^\"|\"$/g, '');
Copy link
Member

@chancancode chancancode Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, this should probably just be argument.replace(/\"/g, '\\"');. Should probably leave a comment saying we are intentionally not wrapping it with quotes (doing it in the template), just escaping any interior quotes.

Suggested change
return JSON.stringify(argument).replace(/^\"|\"$/g, '');
// Escape any interior quotes – we will add the surrounding quotes in the template
return argument.replace(/\"/g, '\\"');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was a side effect of adding JSON.stringify. Before, the quotes were not part of the value.

<span class="value-token">
{{component-argument value}}
</span>
"
{{else}}
<span class="at-token">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my personal opinion is that we probably don't need this 🤔

@RobbieTheWagner RobbieTheWagner changed the base branch from add-args to add-args-2 December 19, 2019 21:03
@RobbieTheWagner RobbieTheWagner changed the base branch from add-args-2 to add-args December 19, 2019 21:04
@RobbieTheWagner
Copy link
Member

Closing in favor of #1112

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.

None yet

3 participants