Page MenuHomePhabricator

Move all comment management junk into a dropdown menu
ClosedPublic

Authored by epriestley on May 3 2014, 8:17 PM.
Tags
None
Referenced Files
F13083194: D8966.diff
Wed, Apr 24, 10:28 PM
Unknown Object (File)
Fri, Apr 19, 4:35 PM
Unknown Object (File)
Tue, Apr 16, 7:10 PM
Unknown Object (File)
Thu, Apr 4, 2:56 PM
Unknown Object (File)
Wed, Apr 3, 12:43 AM
Unknown Object (File)
Sun, Mar 31, 10:21 PM
Unknown Object (File)
Sun, Mar 31, 10:21 PM
Unknown Object (File)
Sun, Mar 31, 10:21 PM
Subscribers

Details

Summary

man I sure hate Javascript

I removed the ajax-edit and ajax-remove interactions, becuase they were prohibitively complex to get working given that the entire menu has to change too. Instead, the page just reloads. This works perfectly fine in practice.

If we want to restore these in the future, we should have the server re-render the entire transaction group or something. I think very little is lost here, though.

Test Plan
  • Took all the actions.
  • Used existing dropdown menus.

Screen_Shot_2014-05-03_at_1.11.50_PM.png (757×897 px, 84 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Move all comment management junk into a dropdown menu.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: btrahan, chad.

Add an "open" state for the icon. Might need a tweak.

{F150200}

  • Slightly smaller changeset.

What's with the icon spacing, these are standard UI elements? Or does this not use PHUIList/ActionList

Oh, sorry, that sounded better in my head. I guess my question is, can we use PHUIList here for some reduction of css/consistency of spacing.

This approach is pretty fragile and the cracks really start to show once I try to introduce PHUIList styles.

I'll just rewrite all the JS stuff, I guess.

epriestley edited edge metadata.
  • Use new dropdown menu stuff.

{F150444}

  • Make these slightly better (?) for assitive technologies, based on me having no idea what I'm doing with VoiceOver.
btrahan edited edge metadata.
btrahan added inline comments.
webroot/rsrc/css/core/core.css
131

sometimes

webroot/rsrc/js/application/transactions/behavior-transaction-list.js
81

will window.location.reload() ever get called?

This revision is now accepted and ready to land.May 5 2014, 4:38 PM
webroot/rsrc/js/application/transactions/behavior-transaction-list.js
81

I should add a comment for this, but in most cases uri is on the same page (just with a different anchor), so browsers navigate to the anchor but don't reload the page. This puts the browser window in the right place, but the user sees the old content.

Explicitly reloading afterward effects "forcefully reload the page, even if it's the current page". This feels a bit iffy and I'd rather just do it on the server side, but we can't always tell what the right detail page is based on a transaction right now (in most cases it's the object monogram page, but in some cases the transaction history is on some separate page), and I don't want to just pass "?redirect=T123#asdf" because semi-open redirects can, e.g., interact with OAuth in dangerous/scary ways and we don't really need to perform one here.

We probably should have some method on Transaction like getTheURIToGoBackToAfterMakingAnEdit(), but then what if there's more than one URI, and probably no one will ever remember to implement that, so despite being a little rough this felt like the most reasonable thing for now to me. I'll make a note about it, though, at least.

epriestley edited edge metadata.
  • Fix "sometimes".
  • Document refresh() stuff.
epriestley updated this revision to Diff 21322.

Closed by commit rPbfc1ccfdf110 (authored by @epriestley).