Page MenuHomePhabricator

Replace the "Choose Subtype" radio buttons dialog with a simpler "big stuff you click" sort of UI
ClosedPublic

Authored by epriestley on Dec 7 2018, 2:15 PM.
Tags
None
Referenced Files
F14110064: D19855.diff
Wed, Nov 27, 4:18 PM
F14105988: D19855.diff
Wed, Nov 27, 4:05 AM
Unknown Object (File)
Sun, Nov 24, 1:33 AM
Unknown Object (File)
Sat, Nov 23, 3:49 PM
Unknown Object (File)
Thu, Nov 21, 12:59 PM
Unknown Object (File)
Tue, Nov 19, 12:59 AM
Unknown Object (File)
Fri, Nov 15, 9:02 AM
Unknown Object (File)
Mon, Nov 11, 3:25 AM
Subscribers
Restricted Owners Package

Details

Summary

Ref T13222. Fixes T12588. See PHI683. In several cases, we present the user with a choice between multiple major options: Alamnac service types, Drydock blueprint types, Repository VCS types, Herald rule types, etc.

Today, we generally do this with radio buttons and a "Submit" button. This isn't terrible, but often it means users have to click twice (once on the radio; once on submit) when a single click would be sufficient. The radio click target can also be small.

In other cases, we have a container with a link and we'd like to link the entire container: notifications, the /drydock/ console, etc. We'd like to just link the entire container, but this causes some problems:

  • It's not legal to link block eleements like <a><div> ... </div></a> and some browsers actually get upset about it.
  • We can <a><span> ... </span></a> instead, then turn the <span> into a block element with CSS -- and this sometimes works, but also has some drawbacks:
    • It's not great to do that for screenreaders, since the readable text in the link isn't necessarily very meaningful.
    • We can't have any other links inside the element (e.g., details or documentation).
  • We can <form><button> ... </button></form> instead, but this has its own set of problems:
    • You can't right-click to interact with a button in the same way you can with a link.
    • Also not great for screenreaders.

Instead, try adding a linked-container behavior which just means "when users click this element, pretend they clicked the first link inside it".

This gives us natural HTML (real, legal HTML with actual <a> tags) and good screenreader behavior, but allows the effective link target to be visually larger than just the link.

If no issues crop up with this, I'd plan to eventually use this technique in more places (Repositories, Herald, Almanac, Drydock, Notifications menu, etc).

Test Plan

Screen Shot 2018-12-07 at 6.03.43 AM.png (1×1 px, 162 KB)

  • Left-clicked and command-left-clicked the new JS fanciness, got sensible behaviors.

Diff Detail

Repository
rP Phabricator
Branch
subtype4
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningwebroot/rsrc/js/core/behavior-linked-container.js:1JAVELIN5`javelinsymbols` Not In Path
Unit
Tests Passed
Build Status
Buildable 21261
Build 28924: Run Core Tests
Build 28923: arc lint + arc unit

Event Timeline

Owners added a subscriber: Restricted Owners Package.Dec 7 2018, 2:15 PM
epriestley edited the summary of this revision. (Show Details)
epriestley added inline comments.
src/view/AphrontDialogView.php
407

The phabricator-remarkup rule has some side effects with margins that prevented the list of options from laying out properly.

This change removes phabricator-remarkup from the body of all dialogs. It's possible this will do some weird things.

I couldn't find any weird things based on prodding it for a bit. When we add remarkup to dialogs, it is normally added inside its own container. I think this was just a legacy thing and that collateral damage will be minor or nonexistent, but this might need some amount of cleanup. In any case, removing this class is clearly more "correct" from a purity standpoint, since dialog bodies are often not remarkup.

src/view/phui/PHUIObjectItemView.php
446–453

This is an existing hack-around for the problem described in the summary, which links the text and separately links the icon in UIs like the /drydock/ console. Since the entire element how acts like a link, I removed this screenreader-hostile secondary supplemental link.

  • Remove some great but ultimately unnecessary debugging code.
This revision is now accepted and ready to land.Dec 10 2018, 7:58 PM
src/view/phui/PHUIObjectItemView.php
452

D19871 corrects the accidental omission of $label here when simplifying the UI element.