Page MenuHomePhabricator

Improve the behavior of PhabricatorFileEditField for Macros
ClosedPublic

Authored by epriestley on May 8 2017, 11:18 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 4:30 AM
Unknown Object (File)
Sat, Nov 16, 6:13 AM
Unknown Object (File)
Mon, Nov 11, 7:52 PM
Unknown Object (File)
Thu, Nov 7, 7:55 PM
Unknown Object (File)
Oct 18 2024, 3:31 PM
Unknown Object (File)
Oct 9 2024, 11:30 PM
Unknown Object (File)
Oct 3 2024, 1:24 AM
Unknown Object (File)
Oct 3 2024, 12:16 AM
Subscribers
None

Details

Summary

See D17848. This improves things a little bit in two cases:

Case 1:

  • Create a macro.
  • Pick a valid file.
  • Pick an invalid name.
  • Submit form.
  • Before patch: your file is lost and you have to pick it again.
  • After patch: your file is "held" in the form, you just can't see it in the UI. If you submit again, it keeps the same file. If you pick a new file, it uses that one instead.

Case 2:

  • Apply D17848.
  • Delete the if ($value) { thing that I'm weirded out about (see inline).
  • Edit a macro.
  • Don't pick a new file.
  • Before patch: error, can't null the image PHID.
  • Afer patch: not picking a new file means "keep the same file", but you can't tell from the UI.

Basically, the behaviors are good now, they just aren't very clear from the UI since "the field has an existing/just-submitted value" and "the field is empty" look the same. I think this is still a net win and we can fix up the UI later.

Test Plan

See workflows above.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

yeah I think I need to design out a full control for view/replace/delete

This revision is now accepted and ready to land.May 8 2017, 11:19 PM

Yeah -- and then we need to wire it up with two tons o' JS.

This revision was automatically updated to reflect the committed changes.

Yell if you catch any weird behavior here, I definitely could have missed something.