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

[DevTools] Support for adding props | Improved state/props value editing #16700

Merged
merged 15 commits into from Sep 10, 2019

Conversation

hristo-kanchev
Copy link
Contributor

@hristo-kanchev hristo-kanchev commented Sep 7, 2019

Solves:

As both issues are highly overlapping, @bvaughn and I decided to submit the fix for both in one PR.

Changes:

  • Extracted sanitizeForParse to the utils file as it's used in multiple places
  • Added new component - EditableName
  • Added support for adding additional entries (currently only enabled for prop)
  • Fixed the issue with locking values by type.
  • String values are now surrounded by quotes.

Screenshots:

Adding new props
props-1
props-2
props-3


Overwriting an existing prop value
props-4


Changing data type - null -> array
props-5

const inputRef = useRef<HTMLInputElement | null>(null);

if (hasPendingChanges && editableValue === value) {
useEffect(
Copy link
Contributor Author

@hristo-kanchev hristo-kanchev Sep 7, 2019

Choose a reason for hiding this comment

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

If the value changes we need to update the stringified version of it.

Copy link
Collaborator

@bvaughn bvaughn Sep 9, 2019

Choose a reason for hiding this comment

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

Copy link
Contributor

@Jessidhia Jessidhia Sep 10, 2019

Choose a reason for hiding this comment

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

Rather, that looks like it should be a useMemo, or even not use a hook at all. You call JSON.stringify for the (unused) initializer value on every render pass anyway.

Copy link
Contributor Author

@hristo-kanchev hristo-kanchev Sep 10, 2019

Choose a reason for hiding this comment

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

I completely removed this bit. Thanks for the input!

let type = 'text';
if (dataType === 'boolean') {
type = 'checkbox';
} else if (dataType === 'number') {
Copy link
Contributor Author

@hristo-kanchev hristo-kanchev Sep 7, 2019

Choose a reason for hiding this comment

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

We don't need this anymore. Now we have the ability to change the value from a number to an array for example.

Copy link
Collaborator

@bvaughn bvaughn Sep 9, 2019

Choose a reason for hiding this comment

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

Is it expected that once a prop becomes a boolean, you can't change it back?
aaaaaKapture 2019-09-09 at 16 42 55

Copy link
Contributor Author

@hristo-kanchev hristo-kanchev Sep 10, 2019

Choose a reason for hiding this comment

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

I thought it would be cool to keep this as it is.
Users can still override it via Add new prop -> type name and set the value to "hello world" for example.
Do you think it would be better if we just displayed true / false instead of the checkbox?
That way we can change it more easily.

Copy link
Collaborator

@bvaughn bvaughn Sep 10, 2019

Choose a reason for hiding this comment

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

Users can still override it via Add new prop -> type name and set the value to "hello world" for example.

That seems pretty non-obvious though. (I don't think it would occur to people to do this).

Do you think it would be better if we just displayed true / false instead of the checkbox?

Yeah 😄 I think we should do this.

Copy link
Collaborator

@bvaughn bvaughn Sep 10, 2019

Choose a reason for hiding this comment

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

I'll just go ahead and make this change.

} else if (dataType === 'string') {
placeholder = '(string)';
} else {
placeholder = 'Enter valid JSON';
Copy link
Contributor Author

@hristo-kanchev hristo-kanchev Sep 7, 2019

Choose a reason for hiding this comment

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

I think it would be good to have this placeholder for non undefined values. What do you think @bvaughn ?

@sizebot
Copy link

@sizebot sizebot commented Sep 7, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 0373472

@davidfurlong
Copy link

@davidfurlong davidfurlong commented Sep 8, 2019

Hey there @hristo-kanchev,
I'd like to weigh in here as well as I was looking at this part of the code and submitted a PR a long time ago that attempted to fix some of these bits. I'm happy to submit a PR to fix the below off of this one or for you to look at these issues

in the KeyValue component

  • New line characters (\n) are hidden from string renders (perhaps hidden characters should be shown but in a different color?)
  • Infinity is rendered as nothing
  • -Infinity is rendered as nothing
  • NaN is rendered as nothing
  • '4' and 4 render as the same (it appears your string change fixes this)
  • empty string renders as (string) when "" is probably better
  • If strings are now in quotation marks perhaps it makes sense for these changes: (null) -> null and (undefined) -> undefined

@hristo-kanchev
Copy link
Contributor Author

@hristo-kanchev hristo-kanchev commented Sep 8, 2019

  • New line characters (\n) are hidden from string renders (perhaps hidden characters should be shown but in a different color?)

From what I can see \n characters are rendered. Or am I missing something?
Screenshot 2019-09-08 at 15 15 37


  • Infinity is rendered as nothing
  • -Infinity is rendered as nothing
  • NaN is rendered as nothing

With this new addition they will be rendered as null do we want to fix this in this PR, Brian?
This is due to JSON.stringify.


  • empty string renders as (string) when "" is probably better

With this PR they will be rendered as ""

Screenshot 2019-09-08 at 15 19 19


  • If strings are now in quotation marks perhaps it makes sense for these changes: (null) -> null and (undefined) -> undefined

With this PR null will be rendered as null but undefined is indeed (undefined)
Screenshot 2019-09-08 at 15 20 51


I think it's up to Brian to decide.
I don't mind adding the support for the above in this PR, but my own personal opinion is that they should be solved in a different issue.

What do you think @bvaughn ?

Copy link
Collaborator

@bvaughn bvaughn left a comment

This is a strong start. Took a review pass and left a few small comments.

|};

export default function EditableKey({
key = '',
Copy link
Collaborator

@bvaughn bvaughn Sep 9, 2019

Choose a reason for hiding this comment

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

key and ref can't be passed in as props though. (They are extracted and treated specially.) So this will never be anything but undefined (or "").

Can we pick a different name for whatever this is? (One that isn't overloaded in React terminology.) I would just suggest naming the state value (and setValue).

I know this is copying the precedent set by EditableValue- but it's borderline mixing controlled and uncontrolled behavior in a way that seems similar similar to this. Maybe we should name the incoming thing e.g. initialValue or something to be more clear?

Copy link
Collaborator

@bvaughn bvaughn Sep 9, 2019

Choose a reason for hiding this comment

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

Hm. Did you mean to use this component for existing keys too? Looks like you're only using it for new keys. Just curious!

Copy link
Contributor

@Jessidhia Jessidhia Sep 10, 2019

Choose a reason for hiding this comment

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

How about calling it name, going by the Object.getOwnPropertyNames nomenclature?

Copy link
Contributor Author

@hristo-kanchev hristo-kanchev Sep 10, 2019

Choose a reason for hiding this comment

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

@bvaughn I initially had this idea and played around with it in the KeyValue component.
It worked pretty well, but it would add additional complexity to this PR and it would require some small tweaks.
Maybe we can create a new issue for this specific case after this get's merged?
What do you think?

inputRef.current.focus();
}
},
[inputRef],
Copy link
Collaborator

@bvaughn bvaughn Sep 9, 2019

Choose a reason for hiding this comment

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

You shouldn't need to specify a ref as a dependency.

let type = 'text';
if (dataType === 'boolean') {
type = 'checkbox';
} else if (dataType === 'number') {
Copy link
Collaborator

@bvaughn bvaughn Sep 9, 2019

Choose a reason for hiding this comment

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

Is it expected that once a prop becomes a boolean, you can't change it back?
aaaaaKapture 2019-09-09 at 16 42 55

const inputRef = useRef<HTMLInputElement | null>(null);

if (hasPendingChanges && editableValue === value) {
useEffect(
Copy link
Collaborator

@bvaughn bvaughn Sep 9, 2019

Choose a reason for hiding this comment

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

@@ -73,6 +122,17 @@ export default function InspectedElementTree({
value={value}
/>
))}
{entryToAdd && (
<div className={styles.AddEntry}>
<EditableKey overrideKeyFn={handleEntryAddName} />:
Copy link
Collaborator

@bvaughn bvaughn Sep 9, 2019

Choose a reason for hiding this comment

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

Is there a way to edit an attribute after it's been added? Looks like it's only editable when it's "new". I guess it's weird to rename a prop being passed in, because a new render would re-add the previous one.

Seems like it might be nice for us to provide a way to edit (or even delete) overridden props though.

Copy link
Contributor

@Jessidhia Jessidhia Sep 10, 2019

Choose a reason for hiding this comment

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

A "reset overrides" button and/or toggle would be a nice debugging feature.

Copy link
Contributor Author

@hristo-kanchev hristo-kanchev Sep 10, 2019

Choose a reason for hiding this comment

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

@bvaughn Regarding the ability to edit a name of an already set prop, as I mentioned in the other comment, I think we should add this support in another PR.

@hristo-kanchev
Copy link
Contributor Author

@hristo-kanchev hristo-kanchev commented Sep 10, 2019

Hey @bvaughn !
I addressed all of your comments.
Unfortunately I'll be going on holidays from tomorrow and I won't be able to add the additional feature requests that you and @Jessidhia asked for.
Feel free to hijack this PR and add them if it's urgent to get them in. If not, I can add them myself next week.

Thanks for the feedback!

@hristo-kanchev hristo-kanchev requested a review from bvaughn Sep 10, 2019
@bvaughn
Copy link
Collaborator

@bvaughn bvaughn commented Sep 10, 2019

  • Infinity is rendered as nothing
  • -Infinity is rendered as nothing
  • NaN is rendered as nothing

With this new addition they will be rendered as null do we want to fix this in this PR, Brian?
This is due to JSON.stringify.

Whoops, sorry. I missed this. Could we use a placeholder for these values?

@bvaughn
Copy link
Collaborator

@bvaughn bvaughn commented Sep 10, 2019

Thanks for the quick turnaround, @hristo-kanchev! I'll review this again this morning and we'll try to at least get an intermediate version landed for now since it's an improvement overall. We can always iterate on it later.

I'll go ahead and make a few adjustments (small things) so the PR won't be blocked. :)

Enjoy your vacation~

@bvaughn
Copy link
Collaborator

@bvaughn bvaughn commented Sep 10, 2019

I think I've got the stringify/parse conditions handling at least the more common values:
Screen Shot 2019-09-10 at 11 10 56 AM

Not sure if we care to support the less common ones (e.g. -Infinity) but we can iterate later.

Copy link
Collaborator

@bvaughn bvaughn left a comment

Thanks! This is a great initial start.

I tweaked a few things (some admittedly subjective). I think we can iterate on this more when you're back, if you're interested!

if (entries !== null) {
entries.sort(alphaSortEntries);
}
const [entries, setEntries] = useState(null);
Copy link
Collaborator

@bvaughn bvaughn Sep 10, 2019

Choose a reason for hiding this comment

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

Curious why this needed to be moved into state? This change would add an additional render (since we sort in an effect now).

I think I'm going to change this back :)

|};

// Convenience hook for working with an editable value that is validated via JSON.parse.
export function useEditableValue(
Copy link
Collaborator

@bvaughn bvaughn Sep 10, 2019

Choose a reason for hiding this comment

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

We could potentially refactor the style editor to use this hook as well, if we replaced the inline calls to smartStringify and smartParse with injected callbacks.

@bvaughn
Copy link
Collaborator

@bvaughn bvaughn commented Sep 23, 2019

I think this change caused a regression (#16859). The <EditableValue> value prop was renamed to initialValue but the HooksTree usage wasn't updated. Flow should have caught this though, and it didn't. That's concerning.

Edit for clarity: I left this comment as a note for myself. I didn't mean for it to sound accusatory at all. Hopefully it didn't! Flow should have caught this. Barring that, I should have spotted this during the review.

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

Successfully merging this pull request may close these issues.

None yet

6 participants