Page MenuHomePhabricator

Add quest objectives to the minimap

Authored by epriestley on May 18 2017, 8:51 PM.
Referenced Files
F10893906: D17955.id43183.diff
Mon, Jul 4, 4:43 AM
Unknown Object (File)
Fri, Jul 1, 4:54 PM
Unknown Object (File)
Mon, Jun 27, 5:20 AM
Unknown Object (File)
Thu, Jun 16, 2:45 AM
Unknown Object (File)
Tue, Jun 14, 5:00 PM
Unknown Object (File)
Sat, Jun 11, 1:25 AM
Unknown Object (File)
Wed, Jun 8, 7:59 PM
Unknown Object (File)
Tue, Jun 7, 5:48 PM



Add important objectives (like waygates and quest markers) to the minimap.

This also probably fixes @cspeckmim's bug with the @ keyboard shortcut.

Test Plan

(This is probably easier to undestand if you arc patch + click around.)

Screen Shot 2017-05-18 at 1.38.56 PM.png (1×179 px, 36 KB)

Diff Detail

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

I talked about this in slightly more detail in T12688, but the specific motivations I have with this are:

First, you can start editing as many comments at once as you want now. Before, you could only edit one at a time. I think the new behavior is generally better and I'd like to keep it, but I expect this to make T3639 (you start typing a comment, do not hit "Save" -- so the comment is still an editable text area, then submit/navigate away) worse, since it's now slightly easer to lose an ongoing edit by forgetting about it while doing a different edit. This provides a visual reminder that you have ongoing edits (currently although it should probably be tweaked so it uses both icon/color), which I hope to help with T3639 (by letting you quickly look for any green in the sidebar before you submit) without requiring us to do something more onerous like popping up a dialog box that says "HEY ARE YOU SURE?" or putting "2 inline comments being edited right now!!" in red in the buoyant header.

I think this may ease the use cases in T8250, including "I want to check off every single inline" and "I want to review only current feedback". You can now quickly see which inlines are "Done" vs not "Done", and click to jump to them. If this sticks, I think it's generally a better direction than having a separate page or copying all the context of every inline into the feed or whatever. We may want to build those approaches as well, but I generally have low hopes for them and would ideally like to try to find a different direction to take.

This feels a little bit "progress bar which you check off in a rote way" to me, but I think it's the least-progress-bar-ish version of this feature that I can come up with.

This might give us some leverage on T11450 by letting us do something very lightweight (for example: starting an inline with "IMPORTANT:" could change the icon to ) to address the use case without requiring each inline to have some kind of additional dropdown or checkbox. I'm very hesitant to clutter the UI with more controls for T11450, which I feel is marginal-at-best.

This element won't scale very gracefully to hundreds of files + hundreds of inlines. Although we can probably improve it a little, that's basically intentional, since you "shouldn't" ever have hundreds of inlines. I don't intend to make Differential a really good tool for making 90 distinct spelling/grammar corrections on a change, per T8909, and want to continue heavily prioritizing the "look at the big picture" use cases when those use cases are in conflict with the "remedy 90 misspellings" use cases.

I'd make un-saved comments orange or pink, some contrast color. I'll patch this in a bit and poke around.

(Generally, I'm not too married to this, but it felt like it might be worth trying while everything else is churning. If everyone hates it or it just causes and endless array of Javascript problems I don't think it serves any critical role in the UI.)

Yeah, that makes sense. Right now it's this, I think:

  • Draft comment which is currently being edited (might be a little buggy in how we're detecting "currently being edited").
  • Draft comment which isn't currently being edited (but which you haven't sumbitted yet).

The "currently editing" should probably become much harder to miss, like . Then maybe drafts should get bumped up a bit.

I suspect some of the ghost/done states could be a little more clear too, and maybe we could group or indicate threads better.

We could also distinguish between "new comment" and "editing comment", although I think those are probably not too different.

Comment objectives don't currently have tooltips, but probably should for at least the self-states ("Editing Comment"). Comments that other users had written could say "authorname" or "authorname on file:line" or "authorname: snippet of comm..". Not sure which (if any) is most useful.

But the current version should at least provide some idea of what a more fully-realized version of this would be like.

I'll accept for now assuming you want this in release. I'll bang on it when I have more time free.

This revision is now accepted and ready to land.May 19 2017, 4:48 PM

I'm a little split on putting it in this release since it feels a bit rough still, but I think it helps smooth over some of the other changes -- a spoonful of visible feature changes helps the refactoring go down. In the worst case we can pull it out or disable it easily later if it turns out it's a giant mess, it's pretty self-contained.

This revision was automatically updated to reflect the committed changes.

Are you tied to this direct or can I break out Photoshop and look into some other ideas? I feel I dropped the ball here in truly understanding what you're building and while. Of course I have two kids' birthday parties to attend today as well.

The direction I'd like to mock out is more "Titan Panel" than "Quest Marker"

Well I was going to see if you still want to land all of this and get all your ideas out. It's helpful for me to see what all it solves in the current state.

I think I like the element, but I'm happy to table it until you have more time to look at things since it sounds like your initial impressions are less positive. D17983 just hides it -- once you have a chance to spend some time on it, we can either turn it back on in master and land all the followups if it seems OK, or pursue something else if you have different ideas, if that sounds good?

I'd like to play around with whatever your full vision is. It's definitely given me a lot of ideas.

If you want to accept all the other stuff I can land them and land D17983 to hide it, then you can comment out the "always hide it line" locally. I don't think any of them do anything bad/complex so even if we rip it out eventually it isn't making a big mess to land 'em.

Any way to gate it maybe to Prototype? I'd like to see it on secure?

Basically, I'm fine with a "turn it off for a while until we have more time to consider things" approach if you'd feel better about that, since it kind of snuck in an hour before release cut.