Page MenuHomePhabricator

Use PHUIListItemView in ConpherenceThreadList
ClosedPublic

Authored by chad on Apr 15 2017, 11:31 PM.
Tags
None
Referenced Files
F14081411: D17698.diff
Fri, Nov 22, 7:41 PM
F14078885: D17698.id.diff
Fri, Nov 22, 5:44 AM
Unknown Object (File)
Tue, Nov 19, 12:17 PM
Unknown Object (File)
Mon, Nov 11, 4:57 AM
Unknown Object (File)
Sat, Nov 9, 1:25 PM
Unknown Object (File)
Oct 23 2024, 4:07 AM
Unknown Object (File)
Oct 15 2024, 3:13 AM
Unknown Object (File)
Oct 13 2024, 4:38 AM
Subscribers

Details

Summary

Fixes T12556 Uses more common components in ConpherenceThreadList by moving to PHUIListItemView. Reduces clutter by moving privacy into the header. Gets ride of "See More" double interchanges.

Test Plan

I need to test this more, doesn't seem to auto-select top room any more, also might build a lipsum generator.

  • Create lots of rooms with various policies
  • Test clicking on policy object
  • Click on different rooms
  • Post in rooms
  • Load up second account, see room numbers
  • Clear room message count by clicking on room

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

These things don't work for me and/or also seem iffy locally:

  • At /conpherence/, I also don't see the top room, just a list of rooms.
  • Going to /conpherence/ and using the left nav just reloads the list, it doesn't select rooms.
  • Stuff does work if I click to a particular room first, then navigate.

Miscellaneous stuff:

  • "Topic • Policies" is a little weird, I'd expect "Policies • Topic" instead? Policies is leftmost in every other application, and topic has to be truncated to keep policies onscreen with this layout?
  • Maximum display topic size feels really small? Lots of unused room with the layout on my monitor.

Screen Shot 2017-04-17 at 6.56.40 AM.png (88×449 px, 18 KB)

  • No way to see full topic if I can't edit the room?
  • UI turns into chaos at tablet sizes:

Screen Shot 2017-04-17 at 6.59.15 AM.png (609×682 px, 121 KB)

  • Search list UI still shows policy icons, maybe now somewhat inconsistent / worth revisiting?

Screen Shot 2017-04-17 at 7.02.40 AM.png (271×133 px, 19 KB)

  • Would be nice for NUX to be reachable via ?nux=trueor similar, ideally with a developer link in the UI, since it's a pain to test as-is.
  • I've got a bunch of T12569 locally:

Screen Shot 2017-04-17 at 7.06.43 AM.png (162×278 px, 8 KB)

  • Maybe ambiguous/confusing that "See All Rooms" takes you to "Joined Rooms", not "All Rooms"? The context where the link exists may also include rooms you haven't joined, if you're looking at a visible but un-joined room. Probably clear enough and I can't really come up with better wording offhand, though.
  • Happy to see "See More" go, always felt kinda weird to me.
  • When I manually set SEE_ALL_LIMIT to 2, I don't actually get a "See All Rooms" link? I think this logic isn't reachable currently, see inline for possible cause.

Screen Shot 2017-04-17 at 7.09.35 AM.png (212×274 px, 21 KB)

src/applications/conpherence/controller/ConpherenceListController.php
37

(Dropping the +1 here might be what's breaking the "See All Rooms" link, since only 20 results ever get loaded?)

webroot/rsrc/js/application/conpherence/behavior-menu.js
428–429

I think this can be removed now.

These things don't work for me and/or also seem iffy locally

Yeah I mentioned this in the test plan, I still need to figure out what I did to break this.

"Topic • Policies" is a little weird, I'd expect "Policies • Topic" instead? Policies is leftmost in every other application, and topic has to be truncated to keep policies onscreen with this layout?

Yeah... don't have a good solution that isn't a ton of work. ie, no CSS only solution give the structure of PHUIHeaderView...

chad planned changes to this revision.Apr 17 2017, 4:15 PM

I'll clean this up.

chad marked 2 inline comments as done.
  • flip order of policy, topic
  • auto-select first room
  • minor css adjustments for widgets
  • better language
This revision is now accepted and ready to land.Apr 17 2017, 6:09 PM

I'm sure this is breaking some edge case I haven't thought about.

This revision was automatically updated to reflect the committed changes.