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

feat(devtools): Allow editing context value in Provider #18255

Closed

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Mar 9, 2020

Summary

Allow editing of context values for modern context types.

Test Plan

Verified locally using packages/react-devtools-shell.

master:
devools-context-master

pr:
devools-context-pr

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 9, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b66f183:

Sandbox Source
falling-shadow-ifpqp Configuration

@sizebot
Copy link

sizebot commented Mar 9, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against b66f183

@sizebot
Copy link

sizebot commented Mar 9, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against b66f183

@eps1lon eps1lon changed the title feat(devtools): Allow editing context value feat(devtools): Allow editing context value in Provider Mar 9, 2020
@gaearon
Copy link
Collaborator

gaearon commented Apr 1, 2020

Can we add some tests for this?

@eps1lon
Copy link
Collaborator Author

eps1lon commented Apr 1, 2020

Can we add some tests for this?

I'd love to but it wasn't clear to me where and how. In previous efforts to work on devtools no tests for the frontend were added: https://github.com/facebook/react/pull/18070/files

@stale
Copy link

stale bot commented Jul 3, 2020

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jul 3, 2020
@eps1lon
Copy link
Collaborator Author

eps1lon commented Jul 3, 2020

bump

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jul 3, 2020
@bvaughn bvaughn self-assigned this Sep 16, 2020
@bvaughn
Copy link
Contributor

bvaughn commented Sep 16, 2020

@rachelnabors brought this to my attention, and I will be happy to review it– after #19774 lands. (I can handle the conflicts in this PR myself if necessary.)

@eps1lon
Copy link
Collaborator Author

eps1lon commented Sep 16, 2020

(I can handle the conflicts in this PR myself if necessary.)

Does it make sense to rebase this PR on your branch at this point? Otherwise I can take care of tending to this PR after #19774 lands if I notice merge conflicts.

@bvaughn
Copy link
Contributor

bvaughn commented Sep 16, 2020

Up to you. I'll likely rebase my branch at least once more after I resolve the last outstanding issue or two. After that, it's up to you if you feel up for handling the merge conflicts or handing them off to me. 😄

@bvaughn
Copy link
Contributor

bvaughn commented Sep 18, 2020

#19774 was just merged. Would you like to fix the conflict in SelectedElement @eps1lon? Or would you like me to? (Happy either way.)

@eps1lon
Copy link
Collaborator Author

eps1lon commented Sep 19, 2020

I'll take a look.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Sep 19, 2020

Looks like the code has completely changed. Would take me a bit longer (over the next week) so if you want to get this in earlier it's probably best if you rebase this PR.

@bvaughn
Copy link
Contributor

bvaughn commented Sep 21, 2020

No worries. I can take a look this afternoon.

@bvaughn
Copy link
Contributor

bvaughn commented Sep 21, 2020

Ah, so to clarify: Editing the context value of a modern context Provider works in master now already.

Editing the context value of a Consumer works in some cases, but not all:

  • You can edit class consumers that use the legacy contextTypes API.
  • You can edit class consumers that use the more recent contextType API.
  • You can not edit modern <Context.Consumer> values. (This would require changes on the backend to support– we'd want to follow the pattern used for editing function component props and inject a new renderer method, and I don't think that's worth it).

Going to close this PR down since it no longer seems to be required. 😄 Thanks!

@eps1lon
Copy link
Collaborator Author

eps1lon commented Sep 21, 2020

Editing the context value of a modern context Provider works in master now already.

Awesome, thanks!

@eps1lon eps1lon deleted the fix/devtools/context-editable branch September 21, 2020 17:07
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.

5 participants