Page MenuHomePhabricator

Transactions - make "show older" area bigger and include keyboard command
ClosedPublic

Authored by btrahan on Dec 4 2014, 10:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 24, 6:22 AM
Unknown Object (File)
Sun, Mar 24, 6:09 AM
Unknown Object (File)
Fri, Mar 1, 12:25 PM
Unknown Object (File)
Fri, Mar 1, 11:19 AM
Unknown Object (File)
Feb 22 2024, 5:13 PM
Unknown Object (File)
Feb 18 2024, 7:34 PM
Unknown Object (File)
Feb 12 2024, 7:54 PM
Unknown Object (File)
Feb 10 2024, 5:04 PM
Subscribers

Details

Summary

Fixes T6683.

Test Plan

clicked the yellow box and it worked! pressed '~' and it worked!

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

btrahan retitled this revision from to Transactions - make "show older" area bigger and include keyboard command.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.

...I figure this gives you another chance to poke holes in this javascript. :D

webroot/rsrc/js/application/transactions/behavior-show-older-transactions.js
108

i need to include this at the top of the file

epriestley edited edge metadata.

I'm not totally convinced that the keyboard command is useful / worth-the-cost, but obviously it's like trivial so whatevs.

Various JS stretch goals inline. Nothing looks bad, but I think a few things could be simpler..


Rather than managing JX.Busy manually, I think you can do something similar to queue() in javelin-behavior-workflow:

var routable = workflow.getRoutable()
  .setPriority(2000)
  .setType('workflow');
JX.Router.getInstance().queue(routable);

This replaces workflow.start(), and manages JX.Busy for you. I think that would let you get rid of loading, too.

webroot/rsrc/js/application/transactions/behavior-show-older-transactions.js
30–31

Is this the same as just calling load_older()?

82–85

The queue stuff could go here, I think -- then remove the .start() calls.

93–96

It might be slightly simpler to pass this .href in on config (like loadURI or something), since the link's href is never actually used/relevant (you can't get anything by clicking the link).

It might hypothetically be nice to make the link a real link to the current page with some parameter like ?xactions=all, then have that load everything when building the timeline. Then this feature would still work (just not quite as nicely) if we break the javascript. The javascript URI for ajax loading could be passed through config.

102–112

Can you simply remove this? Clicks on the link should also be clicks on the block.

This revision is now accepted and ready to land.Dec 5 2014, 12:57 AM
btrahan edited edge metadata.

clean up js. only un-taken suggestion is to move the href into config - this href changes to include the proper pagination key as generated by the server, so changing it strikes me as more complex.

This revision was automatically updated to reflect the committed changes.