Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Card class and migrate card JavaScript to TypeScript #557

Merged
merged 78 commits into from
May 15, 2023
Merged

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Apr 27, 2023

Depends on #555.
Related to #517.
Fixes #558.

This PR adds a new Card class that moves the card initialization JavaScript into TypeScript. I also consolidated the full screen toggle JavaScript into this class, since we were only using it via the card() API.

This PR also brings initialization (#555) and ResizeObserver improvements (#566) from sidebars to cards.

Further improvements to the card component include localized event listeners, rather than relying on a global event listener. This gave us a chance to fix #558 and ensure that we expand the correct card and that we only expand one card at a time – this part is made possible in part by adding a .bslib-has-full-screen to the document body to prevent expanding other cards.

While in full screen, the card component will trap keyboard focus in the full screen area, ensuring that focus doesn't leak to other parts of the document body. I'll add specific tests for this case in follow-up PR in shinycoreci, but for testing you can use the app below.

Tab Focus App
layout_column_wrap(
  width = 1 / 2,
  card(
    full_screen = TRUE,
    card_header("Nothing to focus on here"),
    shiny::p(
      "This is a boring card with just some plain text.",
      "There's something to read here but there aren't any inputs to focus on.",
      "Tabbing will only move focus to the \"Close\" button."
    )
  ),
  card(
    full_screen = TRUE,
    card_header("Inputs, oh my!"),
    shiny::p(
      "Here's a bit of text! This card does have stuff to focus on, and the",
      "first focusable element is automatically focused when the card is expanded.",
      "Try tabbing through the inputs, you can't leave!"
    ),
    shiny::selectInput("letter", "Letter", letters, selected = "a"),
    shiny::textInput("word", "Word", "hello"),
    shiny::textAreaInput("sentence", "Sentence", "hello world"),
    shiny::actionButton("go", "Go")
  ),
  card(
    full_screen = TRUE,
    card_header("A plotly plot"),
    shiny::textInput("search", "Search", "search or something"),
    plotly::plot_ly(x = rnorm(1e4), y = rnorm(1e4))
  )
)

This PR is accompanied by to two PRs in other repos:

  1. Add Shiny Event classes for custom events shiny#3815

    Because we're adding an event handler for a custom Shiny event, "shiny:value", we needed to provide the appropriate overloads to associate "shiny:value" with the ShinyEventValue interface. While looking into that, I refactored Shiny's custom event code.

    This PR no longer relies on Add Shiny Event classes for custom events shiny#3815. In Add ShinyResizeObserver #566 we created ShinyResizeObserver which uses the RO concept first used for cards but without relying on a shiny event.

  2. Update 304-bslib-card shinycoreci#160

    I updated the example card app and modified the tests in shinytest2: 304-bslib-card, in part for easier testing and in part because I moved the listener node for the exit fullscreen event handler. (In short, clicking anywhere outside the full screen card will close the card, like modal with easyClose = TRUE.)

Apply click and transition end event listeners directly to the elements that
need them instead of applying them to the document and relying on event
delegation. We also now use vanilla JS to add and remove event listeners instead
of jQuery, and we remove the event listeners if the layouts are removed the
page.
arrive is a lot lighter and can be included in each component directly
we just use it for the types in bslib components
It's now a part of Card class
1. Properties
2. Constructor
3. Public methods
4. Private methods
5. Static methods
Makes it easier to test without using overly complex selectors
@gadenbuie
Copy link
Member Author

Three biggish changes to callout since the last review:

  • 920a67a Moves all of the tab focus trapping logic into a single method, _trapFocusExit(). This includes more documentation and better naming to document and describe the situations where we're stepping in to ensure expected focus behavior.

  • We now use keydown for all keyboard events, which makes the escape key feel more responsive (6bcb9a8).

  • I improved the special case handling for expanded inputs and open select inputs in 81704a7. Pressing escape while having an open selectInput() or selectizeInput() (both with and without selectize.js) will close the open select dialog but not the full screen card.

  • The Card class is now fully documented with jsdoc (950c789).

Copy link
Collaborator

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@gadenbuie gadenbuie merged commit d3c3ad3 into main May 15, 2023
1 check passed
@gadenbuie gadenbuie deleted the card/js-deps branch May 15, 2023 14:45
@gadenbuie gadenbuie assigned schloerke and unassigned gadenbuie May 15, 2023
schloerke added a commit to posit-dev/py-shiny that referenced this pull request May 15, 2023
@schloerke
Copy link
Collaborator

Ported in posit-dev/py-shiny@f8e5a20

@schloerke schloerke removed their assignment May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple cards expand together in navs_tab_card() and navs_pill_card()
3 participants