Page MenuHomePhabricator

Add watchstars to uniform cards
Closed, DeclinedPublic


Follow up to T114393.


Related Gerrit Patches:
mediawiki/extensions/Cards : masterAdd watchstar

Event Timeline

bmansurov raised the priority of this task from to Needs Triage.
bmansurov updated the task description. (Show Details)
bmansurov added a subscriber: bmansurov.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 26 2015, 5:54 PM
Jdlrobson triaged this task as Low priority.Oct 26 2015, 11:50 PM
Jdlrobson set Security to None.
Jdlrobson added a subscriber: Jdlrobson.
Jdlrobson edited a custom field.Oct 26 2015, 11:58 PM

The following patches need a review:

Feel free to move back to todo once those patches have been reviewed.

phuedx added a subscriber: phuedx.Nov 3 2015, 2:40 PM

@Jdlrobson, @bmansurov: When did you want to talk about 249991? Did you want to talk about it?

Yes, anytime is fine with me.

phuedx added a comment.Nov 3 2015, 5:28 PM

So… What's the intent of the patch? What's the model for and when are the models properties going to change?

bmansurov added a comment.EditedNov 3 2015, 5:46 PM

One use case can be seen here:
When a user interacts with the view (clicks a watchstar icon in this case) all we have to do is to update the model, and listen to model update events, and re-render the view. So, to answer your question, the model properties will change when we want to update the UI after a user input.

I also see models post the new data to the server. Maybe interact with the gateway somehow or replace it completely.

phuedx added a comment.Nov 4 2015, 3:54 PM

So more like Backbone.Model and Backbone.View than MFBone.View?

My main concern around this work is that there is a watchstar in core already. I wonder if this is an opportunity to clean up that code to be a model that can work in multiple instances on a single page and find a way for MobileFrontend to use it.

That said, rendering happens via OO.ui which is a big library to load just for three watchstars. Given the code will be loaded on demand this might not be the worst thing but...

The approach taken is not oojs ui so if we're not doing that we need to acknowledge it and why.

The reasoning behind not using OOJS UI was exactly what you said, it's big. Simple model-view separation seems to get the job done. And it's not like we're creating a full fledged framework there.

I'll have a look at the watchstar code in core. I'd love to use that instead if that's feasible.

This work should be done in parallel. It would be irresponsible to load two pieces of code (one from MobileFrontend and one from Cards) on the same page.

@Jdlrobson started a conversation about the OOjs UI related part of this conversation here:

Change 249957 had a related patch set uploaded (by Bmansurov):
Add watchstar

Pushing this back to in analysis whilst we discuss whether we need the watchstar in a first release (see email thread [Reading-web-team] watchstar on read more)

I'd prefer to see @bmansurov's patch through to completion. I would rather it be the situation that we have a good library – Cards – to use that needs external review and integration than having no library and our current implementations stagnate further.

Me too, I just need to keep us focused to actually deliver this thing.

I will plan for us to wrap this up in December when things have calmed down. Nothing wrong with working on it in the background but right now event logging and getting this feature on desktop is of greater importance.

(and to clarify on the integration part, I think we should be using Cards but for the time being it should simply provide a card without a watchstar)

phuedx edited a custom field.Nov 13 2015, 9:49 AM
phuedx added a subscriber: csteipp.

Following yesterday's security review meeting I've added @csteipp as a subscriber to this card. Chris mentioned that there had previously been one or more issues with handling tokens for watchlist requests and we should reach out to him if we're at all unsure ourselves.

Jdlrobson changed the task status from Open to Stalled.Dec 2 2015, 1:51 AM

We still need to work out how to minimise the size of oojs ui before going down this route.
I might need to bump

Change 249957 abandoned by Bmansurov:
Add watchstar

no need for it for now

bmansurov removed bmansurov as the assignee of this task.Aug 12 2016, 11:04 AM

Not working on it currently.

Jdlrobson added subscribers: ovasileva, Nirzar.

@Nirzar @ovasileva do we want watch stars on related pages or can this be declined?

Nirzar closed this task as Declined.May 21 2017, 11:43 AM