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

5241 - [Preview] Show empty values#5279

Merged
jasonLaster merged 1 commit into
firefox-devtools:masterfrom
Fischer-L:5241-Preview-empty
Feb 2, 2018
Merged

5241 - [Preview] Show empty values#5279
jasonLaster merged 1 commit into
firefox-devtools:masterfrom
Fischer-L:5241-Preview-empty

Conversation

@Fischer-L

@Fischer-L Fischer-L commented Feb 1, 2018

Copy link
Copy Markdown
Contributor

Associated Issue: #5241

Screenshots/Videos

jufkidihcu

Fischer-L
Fischer-L commented Feb 1, 2018
View reviewed changes
Comment thread src/components/Editor/Preview/Popup.js

renderPreview() {
const { value } = this.props;
if (!value) {

@Fischer-L Fischer-L Feb 1, 2018

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.

Found this is a regression from PR #4915
In that PR, added one if (!value) early return[1] so this.renderSimplePreview(value) below for simple typed value, such "", 0, null, wouldn't be reached.

[1] https://github.com/devtools-html/debugger.html/blob/6d4220ecc43e4613f5db8feda6b2c25676f46259/src/components/Editor/Preview/Popup.js#L244

@nyrosmith nyrosmith Feb 1, 2018

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.

Mind leaving a comment so it becomes obvious that there should not be an early return because this breaks things?

@Fischer-L Fischer-L Feb 2, 2018

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.

@nyrosmith That helps in the future. Added

@Fischer-L Fischer-L mentioned this pull request Feb 1, 2018
Closed
@AnshulMalik

AnshulMalik commented Feb 1, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Looking good to me, just need to hack it to please flow,
value.class === "Function" => value && value.class === "Function"

@jasonLaster jasonLaster mentioned this pull request Feb 1, 2018
Closed
@Fischer-L

Fischer-L commented Feb 2, 2018

Copy link
Copy Markdown
Contributor Author

@AnshulMalik

Looking good to me, just need to hack it to please flow,
value.class === "Function" => value && value.class === "Function"

Thanks for reminding this improvement.

@jasonLaster jasonLaster merged commit 1cfe5c2 into firefox-devtools:master Feb 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@nyrosmith nyrosmith nyrosmith left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Fischer-L @AnshulMalik @nyrosmith @jasonLaster