Page MenuHomePhabricator

Make the browse button on the tokenizer, be on the tokenizer
ClosedPublic

Authored by chad on Apr 18 2015, 12:37 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 24, 10:52 PM
Unknown Object (File)
Fri, Apr 12, 5:32 AM
Unknown Object (File)
Fri, Apr 12, 5:32 AM
Unknown Object (File)
Fri, Apr 12, 5:32 AM
Unknown Object (File)
Fri, Apr 12, 5:32 AM
Unknown Object (File)
Thu, Apr 11, 7:47 AM
Unknown Object (File)
Fri, Apr 5, 4:29 PM
Unknown Object (File)
Mon, Apr 1, 7:55 AM
Subscribers

Details

Summary

Moves the Browse... button into a Search Icon on the actual tokenizer. I played with a number of icon treatments, and Search seems to convey the right attribute, other things like lists and menus didn't quite feel right to me, but feel free to push back if you hate search.

Test Plan

Tested lots of tokens, little tokens, small screens, etc.

pasted_file (339×798 px, 56 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

chad retitled this revision from to Make the browse button on the tokenizer, be on the tokenizer.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added reviewers: epriestley, btrahan.
chad edited the test plan for this revision. (Show Details)
chad edited the test plan for this revision. (Show Details)
epriestley edited edge metadata.

If you didn't check them, two other weird test cases are:

  • Maniphest -> Add Subscribers (shouldn't have browse right now, but should work fine).
  • Herald, Maniphest Batch Edit, Edit Policy -> Custom Policy: should have browse and should work (all three of these will probably work fine if any one of them works fine).
This revision is now accepted and ready to land.Apr 18 2015, 4:28 PM

uh, no, those cases don't work... Add CCs now has a button when it didn't previously, the Batch Editor UI is wonky, but I can probably fix that with some CSS.

There's some stuff in tokenizer.css which should probably get cleaned up, too.

Roughly, the idea with the has-browse class is:

  • The button is always rendered into the DOM, just invisible by default.
  • If a control is browsable, we add has-browse: CSS should then reveal the button.

Specifically, some/all of these rules are probably junk now:

.jx-tokenizer-frame {
  width: 100%;
}

.jx-tokenizer-frame .jx-tokenizer-frame-browse {
  display: none;
}

.has-browse .jx-tokenizer-frame-browse {
  display: table-cell;
}

.jx-tokenizer-frame td.jx-tokenizer-frame-input {
  width: 100%;
}

.jx-tokenizer-frame-browse {
  width: 100px;
  vertical-align: middle;
  padding: 0 0 0 4px;
}

(The unusual way the rendering works is because in Herald we produce a single "template" that needs to be able to generate both browsable and unbrowsable typehaeads, so we always have to be able to have a button to show.)

chad edited edge metadata.
  • more edge case testing
  • move tokenizer button css to tokenizer.css
This revision was automatically updated to reflect the committed changes.