RinteRface / shinybulma

🌐 Bulma.io for Shiny
https://rinterface.github.io/shinybulma/
Other
111 stars 15 forks source link

Change interface of `bulmaTabs` #30

Closed thothal closed 3 years ago

thothal commented 3 years ago

In the current implementation, the id tag for the content container is created from the label. This can lead to duplicated id elements:

tabs <- bulmaTabs("tab 1", FALSE, bulmaTab("tab 1", p("placeholder")))

bulmaPage(tabs, tabs)

Secondly, the position of the center argument, makes it necessary that one specifies the center argument even if one does not want to change the default.

In this (API breaking) PR, I propose another interface for the 2 functions, which avoids duplication, assigns "unique" ids and make the usage easier.

DivadNojnarg commented 3 years ago

Thanks for fixing this. Is it impacting other examples?

thothal commented 3 years ago

As said the signature of the function changes, so it may break existing code. However, I strongly believe that this version is more robust as:

So, yes it in impacting (most likely) existing code and should be properly documented. As for R cmd check I think it was the only issue.

DivadNojnarg commented 3 years ago

Looking at the original Shiny api like tabsetPanel and tabPanel, I would prefer not having to specify tab ids in the bulmaTabs. I would prefer something like this:

bulmaTabs(
      bulmaTab("y", p("test")), 
      bulmaTab("z", p("test")))
   )

like:

tabsetPanel(
        id = "hidden_tabs",
        tabPanelBody("panel1", "Panel 1 content"),
        tabPanelBody("panel2", "Panel 2 content"),
        tabPanelBody("panel3", "Panel 3 content")
      )

I merged the first components of the bulma-js branch and bulmaJS supports tabs. Would you be able to look at the API https://www.jsdelivr.com/package/npm/@vizuaalog/bulmajs and the doc https://github.com/VizuaaLOG/BulmaJS/?

thothal commented 3 years ago
bulmaTabs(
      bulmaTab("y", p("test")), 
      bulmaTab("z", p("test")))
   )

That's exactly how I implemented the proposed change. :)

However, unlike tabsetPanel the underlying JS widget (jquery-ui) works by setting the href of the header equal to the id of the content. Thus, we need to find a smart way to come up with unique-like ids. We could force bulmaTabs to take a mandatory id element (to underline the fact that this must be unique), but this is then a bit of an overkill maybe.

tabsetPanel also assigns unique-ish ids and from there I took the approach. Basically they assign a random number (a bit more sophisticated than in my proposal). In the end the user needs not to be bothered about ids o elements which he will most likely never address anyways.

WHat we could think of though, is to provide a optional id arguemnt to bulmaTabs to implement an input$<this_id> reactive, suhc that the user could determine on which tab she is (like for tabsetPanel.

thothal commented 3 years ago

I think I messed up with my git repo. Here is a commit which should be undisputed 77cea02 and it should have been part of the fix_tabs branch.. Basically it also hides the elements when we leave a tab. I am not very experienced with git yet (as you can see), but is there a chance to get this commit into master as it actually belongs to the fix_tabs branch?

Because this commit should be part of the prior change and has nothing to do with the proposed API change here.

DivadNojnarg commented 3 years ago

My bad! I looked at the wrong example. I'll solve your git conflict. I am not against an input binding to detect the currently selected tab. If you like to to it, I'm happy to review your code in another PR