Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Initial Components pane for React#6018

Merged
jasonLaster merged 4 commits into
firefox-devtools:masterfrom
iocalebs:react-components-pane
Apr 19, 2018
Merged

Initial Components pane for React#6018
jasonLaster merged 4 commits into
firefox-devtools:masterfrom
iocalebs:react-components-pane

Conversation

@iocalebs
Copy link
Copy Markdown
Contributor

Related issue: #3792

Summary of Changes

  • Add a "Components" pane that appears when pausing in a React component. For now, it just lists the component and all its ancestors all the way up to the root.
  • This is hidden behind a feature flag for now.

Todo in a future PR:

  • Ability to "select" components. Right now it's just a plain list.
  • Improve styling?
  • Improve handling for container components created with connect?

Test Plan

  1. Hit a breakpoint at Frame/index.js#174
  2. Observe the "Components" pane as in the screenshot below

Screenshots/Videos

image

Comment thread src/actions/pause/extra.js Outdated
if (!props.react) {
props.react = await getReactProps(evaluate);
}
props.react = await getReactProps(evaluate);
Copy link
Copy Markdown
Contributor Author

@iocalebs iocalebs Apr 18, 2018

Choose a reason for hiding this comment

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

@jasonLaster @mmcote Is this OK that I remove this stuff? It was the easiest way to get what I wanted and it didn't seem necessary. I can put it back if needed ofc.

Copy link
Copy Markdown
Contributor

@jasonLaster jasonLaster left a comment

Choose a reason for hiding this comment

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

Yessss

Comment thread src/utils/prefs.js
pref("devtools.debugger.features.outline", true);
pref("devtools.debugger.features.column-breakpoints", true);
pref("devtools.debugger.features.replay", true);
pref("devtools.debugger.features.component-stack", true);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

these flags will need to be added to assets/prefs as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed in ca230f1

Comment thread src/components/SecondaryPanes/index.js Outdated

getComponentStackItem() {
return {
header: "Components",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lets add an L10N.getStr property here

Copy link
Copy Markdown
Contributor Author

@iocalebs iocalebs Apr 19, 2018

Choose a reason for hiding this comment

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

fixed in 156ba1e

Comment thread src/actions/pause/extra.js Outdated
if (!props.react) {
props.react = await getReactProps(evaluate);
}
props.react = await getReactProps(evaluate);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we need to keep this, but we can refactor it...

for instance always extends props.react... not do that !props.react check

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed in ca230f1

Comment thread assets/panel/debugger.properties Outdated
# message to show more of the frames.
callStack.expand=Expand rows

# LOCALIZATION NOTE (components.header): Components right sidebar pane header.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe something a bit more descriptive? e.g. 'Header pane in the right sidebar for React Components', assuming it's only for React

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How's this? f2c5442
'Header for the React Components pane in the right sidebar.'

For now, it's only for React.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That works for me.

componentNames.result.preview && componentNames.result.preview.items;
if (items) {
return {
displayName: items[0],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm, i'd prefer not overriding displayName. The rationale is if you are debugging a site that is minified, but has source maps looking up the class name from the symbols table is not minified. Crazy, right?!?!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! Fixed in 106eeaa

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

actually wait 106eeaa still changes the behavior... bc60352 should do the job.

Comment thread src/actions/pause/extra.js Outdated
if (!props.react) {
props.react = await getReactProps(evaluate);
}
merge(props.react, await getReactProps(evaluate));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we use splat here {...props.react, ...await getReactProps(evaluate) }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure can. 106eeaa

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

update: bc60352

@jasonLaster
Copy link
Copy Markdown
Contributor

@calebstdenis can you rebase and fix the the circle tests

Comment thread assets/panel/debugger.properties Outdated
# message to show more of the frames.
callStack.expand=Expand rows

# LOCALIZATION NOTE (components.header): Header for the React Components pane
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be Header for the Framwork Components

we want to add Ember soon :)

Appears when pausing in a React component. Lists the component and all its ancestors up the root.

The intention is to eventually have something similar to the call stack but for React component hierarchies.
ReactComponentStack is to the CallStack what FrameworkComponent is to
the Scope pane.
I would've liked to remove mocking from the tests entirely to make them
more robust, but my attempts at that failed.
@iocalebs
Copy link
Copy Markdown
Contributor Author

Had to fix some Jest tests 487edd6

@jasonLaster jasonLaster merged commit 0892092 into firefox-devtools:master Apr 19, 2018
jasonLaster pushed a commit to jasonLaster/debugger.html that referenced this pull request Apr 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants