Page MenuHomePhabricator

Add support for future toasts
Closed, ResolvedPublic

Description

In some workflows a page reload is needed before a toast is shown
e.g. editing, deletion of a Gather page.

The toast module should support showing toasts later e.g. toast.showOnPageReload() and we should all use the same method to do this.

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task to Triaged but Future on the Web-Team-Backlog board.
Jdlrobson subscribed.

Change 209777 had a related patch set uploaded (by Bmansurov):
Add support for future toasts

https://gerrit.wikimedia.org/r/209777

Ping @Jhernandez would be good to get your input on this.

Bringing the comment from Gerrit::

@bmansurov This was merged some a couple of weeks back in gather:

https://github.com/wikimedia/mediawiki-extensions-Gather/tree/master/resources/ext.gather.alerts

This is the module that depends on toasts and that defines the add and show function https://github.com/wikimedia/mediawiki-extensions-Gather/blob/master/resources/ext.gather.alerts/futureToasts.js

And this is the file included in every page where you want to show the previously queued toasts : https://github.com/wikimedia/mediawiki-extensions-Gather/blob/master/resources/ext.gather.alerts/init.js

This patch is pretty similar, one difference I see is that the gather version would allow to queue several toasts.

Not sure what to do but it would be good to sync on this somehow. Either make gather use this version or move gathers version to mobile frontend and make gather use it.

Whatever you guys consider.

I also left a comment in gerrit. Let's continue discussing it there. Can @kaldari, @phuedx, @Jdlrobson share their opinions?

It's a shame about the duplication as now we have 2 solutions which look fine and it's not clear which is the right one. I think ultimately it will come down to who does the work first so I guess that's yours baha :-)

I agree with @Jhernandez though we should be using the same code. If Gather could be updated in the same change this will give validation that we've landed on a generic solution that fits everyones needs.. that why this bug was setup after all since nothing is currently broken we just need a standard method for doing this.

It's a shame about the duplication as now we have 2 solutions which look fine and it's not clear which is the right one.

With this kind of work it's likely that duplication of efforts would occur. It's an innocuous task that's quite small.

@Jdlrobson: I feel like it's our responsibility to do the due diligence of syncing around these tasks, to see if there's already a solution for example, before we line 'em up for development.

Regarding which patch gets merged: it feels like Gather should be using functionality provided by MobileFrontend here, so either @Jhernandez reviews and merges @bmansurov's existing patch or @Jhernandez submits a patch to upstream his existing work and @bmansurov reviews it.

Want to flip a coin?

Agreed @phuedx but it seems this was not originally in your sprint? Seems like we need to be more disciplined in how non sprint work is taken care of. I know I'm bad at this sort of thing for example... Which I'd say is due to tech debt not getting attention it deserves and being done on the side.

Good to see some discussion on this. I'd like to point out that there is a fundamental difference in functionality as pointed out in the gerrit commit. Feel free to respond there.

I would suggest we go with Baha's patch but Baha should update the Gather code to use his patch. That way we validate it by showing it has generic use. The bug was about providing a standard interface so from my perspective without the gather patch the task hasn't been completed.

Change 211024 had a related patch set uploaded (by Bmansurov):
Use MobileFrontend toast to show toasts on page reload

https://gerrit.wikimedia.org/r/211024

Change 209777 merged by jenkins-bot:
Add support for future toasts

https://gerrit.wikimedia.org/r/209777

@Jdlrobson, @Jhernandez: Now that the MobileFrontend patch has been merged, I'd really appreciate your help getting this resolved.

@bmansurov your patch looks good to me - just needs a rebase.

Change 211024 merged by jenkins-bot:
Use MobileFrontend toast module to show toasts on page reload

https://gerrit.wikimedia.org/r/211024

Jdlrobson moved this task from Ready for signoff to Done on the Gather Sprint Help! board.