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, Apr 24, 10:12 PM
Unknown Object (File)
Mon, Apr 22, 2:03 PM
Unknown Object (File)
Thu, Apr 11, 7:31 AM
Unknown Object (File)
Mar 23 2024, 5:03 AM
Unknown Object (File)
Feb 22 2024, 9:57 AM
Unknown Object (File)
Feb 22 2024, 8:43 AM
Unknown Object (File)
Jan 24 2024, 3:38 PM
Unknown Object (File)
Dec 27 2023, 7:52 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.