Move landing popup feature back to the modules argument

[pawelru]

Recently we have moved the landing popup feature to become one of the arguments of init(). I don't think it's correct. If you compare the relevance of all the other arguments including modules or data - this is clearly not a good fit. Conceptually this should remain as a module. Let's move it back.

[m7pr]

But we also have header, footer and title which are of the same importance as landing page

[pawelru]

But we also have header, footer and title which are of the same importance as landing page

I think I would disagree with that statement. This is just a popup which once closed - won't appear anymore. On the other hand, header, footer etc. is a persistent UX element visible throughout the whole user journey through the app. This is why I personally consider this as a less relevant.
Moreover, these two arguments were there in init() from the very beginning. It would be a significant breaking change if we move it elsewhere. This is completely different to the landing_popup argument introduced just yesterday (and obviously: not yet released).
In addition, the landing popup is a module - link. Why do we keep it separate from all the other modules?

All in all, we have the following arguments that should be seen as follows:

• data - data or the module returning the data
• modules - app contents
• header, footer - app UI

The landing popup functionality plays very nicely with the responsibility of the modules argument. I don't really understand the motivation behind that recent change.

Why do I care? It is a well known fact that teal as a whole might be overwhelmingly complicated. Especially for a new R user transitioned from other programming languages like SAS. Our goal is to keep it simple(r). One of the very first function that they will see is init() and I would like to keep it simple(r). That recent change was the opposite.

[m7pr]

Landing module is a popup, and we had no control over controlling multiple popups that could be created with modules (logging popup). So to prevent the behavior that landing popup does not work with other modules involving popups we decided to move it to the init. I hear your points about app UI. Maybe to make it even simpler we can consider grouping header,footer,title in a list called ui/ui_options and limit the number of parameters even more. Landing popup could be there as a welcome message popup. Even it's a module, it doesn't really serve the purpose of the module

[pawelru]

Thanks for the explanation. It looks to me that the implemented solution of the issue you described created a new issue. For me, even worse one as this is even more visible - actually the most visible as it could be as this modifies the top function from the package which we should be extremely careful about.

---

Even it's a module, it doesn't really serve the purpose of the module

Yes and no. This is true that it's not similar to all the other modules but on the other hand out of the top-level arguments we have, this fits the most into the "modules" argument.
I see modules argument as a content of the app. Or list of screens in the app. The landing popup is just a div-less screen.
I do get that the landing popup is a little bit unusual module because it has to be a singleton and this requires some additional logic implemented. But see my previous point: not to replace one issue with another (even worse) one.

---

Maybe to make it even simpler we can consider grouping header,footer,title in a list called ui/ui_options and limit the number of parameters even more.

I have a similar thought but see my previous message:

these two arguments were there in init() from the very beginning. It would be a significant breaking change if we move it elsewhere.

Also, it does not solve the landing popup issue. It's a separate issue though. Let's try to stick to the landing popup topic.

[m7pr]

I have a similar thought but see my previous message:

these two arguments were there in init() from the very beginning. It would be a significant breaking change if we move it elsewhere.

We can always have the depracation period. We can introduce UI parameter that will be a list, and this will handle welcome/landind_popup and title,header,footer and in the same time we still support title,header,footer parameters during the depracation.

For the landing popup I think we might need a bigger team discussion as I don't feel authorised to speak for everybody in this matter

[gogonzo]

this is clearly not a good fit.

I disagree. I think it is not a good fit to include not teal_module in the modules argument. I don't think landing_popup being a "teal_module" is correct. Here are reasons:

1. What if somebody adds two landing popups? This most probably needs to be a single module
2. landing_page_popup doesn't have datanames nor doesn't use transformers. It is just ui and server
3. landing_page_popup is not displayed as a tab

P.S.
I also have an opinion that reporter_previewer is not a "teal_module". With excluding objects from modules argument we achieving clear separation, what is a teal_module (tab) and what are additional features of teal (reporter, landing_page_popup etc.)

[vedhav]

There are two broad ways in which teal can be used:

1. Teal as a standalone app using teal::init
2. Teal as a shiny module using ui_teal and srv_teal

If the landing popup is part of the modules, we allow the possibility to have multiple landing popups inside one shiny app when people choose to create a shiny app using method 2. When this happens only one popup will be visible.
So, it makes sense to have this as a "teal app" feature instead of a "teal module" feature, and a good way to achieve this is to have it as a parameter to init.

[m7pr]

@pawelru we are getting back to this discussion as this is a potential blocker for next teal release insightsengineering/nestdevs-tasks#66

What is your current opinion on the matter? Do the proposed arguments changed your attituted?

[pawelru]

Oh it was quite a while ago. Skimming through the comments I have to admit that not really - unfortunately. Let's try to organise a quick chat about that with whoever wants to contribute - it would be faster to make a call.

[gogonzo]

I'm open for a discussion involving @donyunardi and @kumamiao

[m7pr]

@donyunardi would you mind scheduling a call this or next week? If we align on the approach, I think we can then account for the result of the discussion during the PI Planning

[m7pr]

I sent an invite for next Monday, past refinement

[m7pr]

Hey team, thanks for the meeting yesterday! I think that's a good opportunity to restart the conversation.
We all agreed that teal::init should not have extra parameters if they are not that important as current data, filters or modules.
We talked about the possibility to extract reporter_module of of modules as well and put it as an extra parameter of init.
We discussed possibilities to remove title, header and footer out of init parameters as they are not that important, even though they have been there from the very beginning.
We talked also about potential customized wrappers that could be applied on init that could edit server and ui code.

One of the propositions was to reshape init, so it takes below

init(data, filters, modules, reporter)

and every other customization, like server additions, footer, custom JSS, landing popup, could be custom wrappers

• add_langing_popup
• add_title
• add_footer
• adjust_server

[donyunardi]

Init should have only limited arguments: init(data, modules, filter, reporter).
We'll keep the init(id=) argument for now and discuss it on a different issue.
We'll also discuss about adding the reporter argument in a different issue.

For this issue, we'll focus on landing_pop, header, title, and footer argument.

Acceptance Criteria

• Create custom wrappers for deprecated arguments (header, title, footer, landing_popup).
• Change the deprecation message is in the module (because we haven't release when we move landing_popup to init).
• Remove the landing_popup from init directly (because we haven't release when we move landing_popup to init).