Better add blocks

[DivadNojnarg]

This is similar to #325 but with 0 JavaScript (for easier customisation):

• Add new category field to the registry: allows to sort blocks on the UI side. Default to uncategorized to avoid breaking change.
• Add get_registry: renders as a dataframe for easier manipulation than available_blocks().
• add_block_server is now a real module: it uses get_compatible_blocks that return a list of block compatible with the last stack block. This helper func can also be used without shiny. Block choices are rendered server side as a consequence of running get_compatible_blocks that listens to any block change and update the list of available blocks. The search/select input leverages shinyWidgets's virtual select.
• On the UI side, add_block_ui is now a real UI module function.
• Add remove_stack_server (code cleanup).
• Fix tiny issue on the server side when removing a block from a stack: some reactive values were not updated correctly.

[DivadNojnarg]

@nbenn This is good for review.

Other than that, the decision to drop the JavaScript from PR #325 is to have all the logic into one place for easier customisation, like we already discussed. If for instance, people are note happy with add_block_ui and add_block_server, they can write their own and that would work.

[DivadNojnarg]

@JohnCoene, @karmatarap and @nbenn preview of how it works:

How it looks for a single stack with 2 block packages loaded

Screen.Recording.2024-08-27.at.08.48.14.mov

For a workspace (display of the result block)

Screen.Recording.2024-08-27.at.09.59.52.mov

[JohnCoene]

Is there any way to make the category not a breaking change?

[DivadNojnarg]

@JohnCoene Added a default "uncategorized" for category to register_block and register_blocks. This is what you would get without doing anything:

[JohnCoene]

On an empty stack I see

Where I cannot add the codelist_block we created for the DPP.

https://github.com/BristolMyersSquibb/blockr.application/blob/f6fd05e5e6112ed7df8dd31561149baca529f64a/R/register.R#L12

[DivadNojnarg]

Either we have to explicitly pass category = data to the block to be considered as an entry point, or we need to add some logic that checks if the block input is NA, and assumes it can be seen as a data block by adding the data category on the fly. Maybe the second approach is too invasive and would have unwanted effects?

Can you try:

blockr::register_block(
    new_codelist_block,
    "Dummy data",
    "Create dummy data",
    input = NA_character_,
    output = "list",
    category = "data",
    package = pkgname,
    classes = c("codelist_block", "data_block")
  )

[JohnCoene]

But how does the logic for what block can be added work? The block is valid since the input is NA, I would expect it to appear but maybe under "uncategorised".

[DivadNojnarg]

Until now, the logic was just looking at whether the block belonged to the data category. If un-categorized, even though the input was NA, nothing was made. I assume the assumption that when input is NA, the block is an entry point seems fair, so we don't need to rely on the category for this. I added some logic to handle this, which does this if the data block is labelled as un-categorized (or any other label):

[JohnCoene]

@DivadNojnarg Thank you, seems to work fine now. I will test some more. One thought is whether we need the "+" button, it feels like a needless click, shouldn't we just have to select the block we want (rather than select block then click "+")?

You may also want to test it with a lot of blocks to see performances (should be OK) and see how they display, it needs overflow-y to allow scroll.

[DivadNojnarg]

Thanks for the feedback. I've been experimenting a minimalistic approach:

• Having a search/select element: the searchable items are populated server side as soon as the stack changes (adding or removing a block). Selecting the element adds it to the stack without needing an extra add button.

Screen.Recording.2024-09-03.at.15.47.12.mov

Advantages:

• Search and select all in 1.
• Virtual select, more performant with many blocks.
• Less R code.
• Works from the keyboard (search and press enter works).

Drawbacks:

• Relies on the shinyWidget virtual select input (1 extra package dependency).
• Description field is rather limited (no tooltip or popover).

What do you think @JohnCoene, @nbenn and @karmatarap ?

[DivadNojnarg]

@nbenn Good for final review ;)

[JohnCoene]

I honestly prefer how it was before the search was added.

We don't use any of the space we have to display blocks: the select input is a small box and scrolling through is rather unwieldy.

Is there a way to make that fit the width and height of the offcanvas?

[DivadNojnarg]

Yes the width can be set to 100% of the offcanvas and we can display much more blocks on the vertical space. How many blocks would you like to see at once? 10? That's also configurable.

That would give this:

[DivadNojnarg]

@JohnCoene. We can add custom renderers for the search items, to make them a bit nicer to look. This can also be useful to render thumbnails later, or any other registry information.

Even extra tooltips and popovers.

[JohnCoene]

Should we automatically close the offcanvas after having selected a block?

[nbenn]

Lgtm. Some minor comments added.

Not for this PR, but something to start thinking about: How can we make it possible to add arbitrary further attributes to the registry? High level thoughts:

• register_block(), new_block_descr(), ... would take ...
• register_blockr_blocks() would run conditionally (on some blockr.auto_register_blocks option
• the wrapping package can then provide its own .onLoad() hook to pass the desired attributes to register_block() et al.

What am I missing? @DivadNojnarg is there anything you might want to do differently here in anticipation of such an extension?

[nbenn]

Is there a reason for not moving "category" further down the arg list (given that we provide a default)?

[DivadNojnarg]

It was more a semantic reason, I thought category might be more important than the pkg name but than can move at the end.

[nbenn]

Same comment as above.

[nbenn]

Do you need to expose a name for the constructor here? The names here are just the names used for constructor bindings in the block_registry environment. If a result_block is called result_block in this environment, this is to be treated more as a coincidence than a "rule".

[DivadNojnarg]

That's an artifact I needed for the accotdions and pills but we don't need anymore with the new virtual select. To cleanup.

[nbenn]

This does not look super robust to me: the name for this binding (within the block_registry env) is not part of any API really.

Also, we're hard-coding here some implicit assumption that has to be satisfied for some block class. Maybe this is beyond the scope for this PR, but what if a 3rd-party block implementation also would want such kind of special treatment? What kind of API could we offer to enable this? And if that sounds too complicated, is it worthwhile to do something (hacky) like this for the result block only?

[DivadNojnarg]

We need a separate discussion on the registry. That goes beyond this PR.

[nbenn]

Again, many implicit assumptions here: it is not guaranteed in any way that the (first entry) of a block class attribute is identical to the binding name of the block constructor in this lookup environment.

Why not compare the (full) class attribute to the classes registry "attribute"? That should be way more stable, no?

[DivadNojnarg]

That would work. However, I find not optimal that we have to define the block classes within the constructor:

new_block(
    fields = fields,
    expr = quote(.(stack)),
    ...,
    class = c("result_block", "data_block")
  )

and that we have to do the same for the registry definition. But, again, we need to discuss about the registry separately.

[DivadNojnarg]

Should we automatically close the offcanvas after having selected a block?

I depends if you want to be able to add multiple blocks quickly by chaining the selection, which is possible right now. In that case, closing the offcanvas each time would be annoying.

[DivadNojnarg]

@nbenn Thanks for the comments. I fixed some issues, and also made the add_block stuff generic in case we want to support different options.

[nbenn]

Lgtm.