Page MenuHomePhabricator

[web] Move `errorMessage` state up from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`
ClosedPublic

Authored by atul on Apr 18 2022, 10:25 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 5, 6:03 AM
Unknown Object (File)
Wed, Oct 23, 10:56 PM
Unknown Object (File)
Thu, Oct 10, 2:51 AM
Unknown Object (File)
Thu, Oct 10, 2:50 AM
Unknown Object (File)
Thu, Oct 10, 2:50 AM
Unknown Object (File)
Thu, Oct 10, 2:50 AM
Unknown Object (File)
Thu, Oct 10, 2:50 AM
Unknown Object (File)
Thu, Oct 10, 2:48 AM

Details

Summary

Move errorMessage out of the inner class component state, and move it out to the wrapping "Connected" functional component.

As part of the work to refactor ThreadSettingsModal… specifically to turn it into a functional component.

Test Plan
  1. Kill the node server
  2. Try to save a change to thead settings
  3. Observe that error message appears as expected

83b8.png (1×712 px, 79 KB)

Note: The error message styling (showing up to the right of the button) isn't great. That's "on the todo list" and will be handled once the "noop" refactorings are complete.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul retitled this revision from [web] Use `React.useState('')` for `errorMessage` in `ThreadSettingsModal` to [web] Move `errorMessage` state up from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`.Apr 18 2022, 10:26 AM
atul requested review of this revision.Apr 18 2022, 10:30 AM
ashoat requested changes to this revision.Apr 18 2022, 9:14 PM

Why is this necessary? Each diff should "stand on its own" and be reviewable without additional context. "As part of the work to refactor ThreadSettingsModal" is not enough of an explanation for me to understand why this state needs to be lifted. If the explanation is in another diff, a simple link to that diff should be enough!

This revision now requires changes to proceed.Apr 18 2022, 9:14 PM

(Please apply the above feedback to every diff in your stack to which it applies)

atul requested review of this revision.Apr 18 2022, 10:01 PM
atul edited the summary of this revision. (Show Details)

Cool, that's helpful. Please edit the diff description of every diff to include that info!

This revision is now accepted and ready to land.Apr 18 2022, 10:02 PM
This revision was landed with ongoing or failed builds.Apr 19 2022, 11:29 AM
This revision was automatically updated to reflect the committed changes.