Code review comment for lp:~brian-murray/launchpad/limited-subscriptions-page

Revision history for this message
Guilherme Salgado (salgado) wrote :

On Mon, 2010-09-20 at 13:52 +0000, Deryck Hodge wrote:
> Hi, salgado and sinzui.
>
> We do have a LEP for this feature. This +subscriptions page is one
> bit of the "Better Subscriptions and Notifications" story we're
> currently doing development work on. The LEP is at:
>
> https://dev.launchpad.net/LEP/BetterBugSubscriptionsAndNotifications
>
> This is an old LEP now, and I think we've refined the process a bit
> since writing this one, so maybe some elements are missing that should
> be there. This is not Brian's fault, so please don't block Brian for
> this reason. Let me know if you need something there, and I'll add
> it.
>
> The link Brian provided is the result of the user research we did with
> mrevell for all of the UI elements within this story.
>
> https://dev.launchpad.net/Research/Bugs/Subscriptions2010
>
> A bit of history here may help with some of your concerns, too. We
> did try to work with the DUX team on this feature. Francis, Jono,
> Tom, Graham, and I did a fair amount of coordination work, and in the
> end we spent two months chasing meetings, which slowed the progress of
> this feature. We did get valuable feedback from a couple sessions
> with Alejandra about subscriptions UI elements, and this
> +subscriptions page is a direct result of that feedback. We also then
> did two rounds of user testing on these mockups with mrevell. I think
> we've more than covered ourselves in terms of thinking about the UI
> before beginning to implement it.

Cool, that's good to know. I asked for a LEP because I'd expect it to
either contain some of this information or at show me that this UI (and
possibly others) had been thought through. By looking at the merge
proposal this was not clear.

>
> Having said that, I don't think https://dev.launchpad.net/UI/Design
> should be consider the gospel for UI procedure, at least the initial
> pre-imp recommendation in its current form. We on the bugs team in
> particular are trying to experiment with approaches to UI design to
> achieve quicker turnaround on both producing mockups and receiving
> feedback. I think a pre-imp with a UI reviewer should be considered
> the same as for a code reviewer -- recommended, but the
> designer/programmer can use his/her own discretion. We also cannot
> block on speaking with the DUX team as my team has already proven this
> will delay work beyond a reasonable amount of time. Mockups, of
> course, are a reasonable requirement. Regardless, in this case, we've
> covered all our bases. ;)

I completely agree.

Someone (I think Curtis) said that UI reviews are more about making sure
people think carefully about UI changes rather than anything else, and
that's what I was trying to do by pointing to the wiki page. Again, the
merge proposal had no indication any of this was done.

>
> Finally, as to some of your more technical concerns. I have asked
> Brian to land this page without any Ajax enabled widgets initially.
> The subscription widget is not easily re-usable, and it would require
> one or two more branches to get widgets enabled for this page. This
> is meant to be the very first step in this process for Brian. He has
> many more branches to go until this work is completed -- adding Ajax
> widgets, adding sections for every item we support a subscription on,
> good paginating for these things, performance tuning every step of the
> way, and finally making a link available to users so they become aware
> of this page. I would prefer we don't add a link and hide it with a
> feature flag since you guys are correct that we haven't yet thought

Fair enough. That sounds like a good plan to me.

> about where this link should go. I think there is value in telling a
> few key stakeholders about this page as we work on it, and this seems
> simpler to me than feature flagging it, at least at this point in the
> life of the feature flag system. If we do require this, let's have
> those discussions and have Brian land the feature-flagged link in his
> first follow-up branch.

At this point I don't think using a feature flag buys us much, so I'm
happy with just not linking to the page anywhere.

>
> So given all this (sorry for length), what does Brian need to do to
> get this landed? As I read the concerns above, this would mean:
>
> * Use the suggested MixIn class to get returned to the referring page
> correctly
> * Clear up the icon usage on the page (edit vs. remove operations)
> * Clear up usage depending on the user viewing the page (i.e. if my
> page, provide edit/remove links, if not my page, do not provide
> edit/remove links)

Yes, given your plan, that would be good enough.

> * Refine the user story page test a bit

Or write just unit tests until, as you mentioned on IRC.

>
> We would not yet worry about:
>
> * determining where the link leading to this page goes
> * feature flagging such a link or the page itself
> * using AJAX widgets
> * developing a full and accurate user story page test
> * any UI inconsistencies that will be fixed in later branches (like
> the confusing experience when landing on the +subscribe page depending
> on context and user)
>
> salgado and sinzui, does this seems reasonable to you guys? Or do you
> have other concerns that Brian should address in order to get this
> ready to land?

Sounds good to me.

 review needsfixing

review: Needs Fixing

« Back to merge proposal