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

fix(306,308,310): Sidebar and accordion updates #231

Merged
merged 25 commits into from
Oct 31, 2023

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Oct 23, 2023

Minimizes transitions in sidebar by setting

.bslib-sidebar-layout {
  --bslib-sidebar-transition-duration: 5ms;
};

It turns out we need a little transition time or the final part of the sidebar state transition lifecycle doesn't happen.

TODO

  • 308-sidebar-kitchen-sink
  • 310-bslib-sidebar-dynamic
  • 308-sidebar-kitchen-sink top padding on mobile
  • Can the changes in this PR also fix 306

@gadenbuie
Copy link
Member Author

gadenbuie commented Oct 24, 2023

This will still exhibit failures for the following reasons.

✅ 310-bslib-sidebar-dynamic

image

Nested sidebars on mobile don't account for the taller space requirements of the toggle.

✅ 308-sidebar-kitchen-sink

The flakiness in this test seems to be a problem with how chromote/Chrome take screenshots. Here's a screen recording of the screenshot process for the first tab, slowed down to 1/16 speed. You can clearly see some re-layout happens and it's very likely the screenshot happens just before the layout settles.

shinytest2-screenshot-reload-slow.mov

It looks like shinytest2 defaults to delay = 0, whereas chromote's screenshot() method uses delay = 0.5. I upped the delay in 308 to 0.25 with

AppDriver$new(
  variant = platform_variant(),
  name = "308-sidebar-kitchen-sink",
  # ...
  # Set a delay to screenshot after layout is settled
  screenshot_args = list(delay = 0.25)
)

@gadenbuie gadenbuie self-assigned this Oct 24, 2023
@gadenbuie
Copy link
Member Author

I finally figured out 308 -- I disabled the sidebar transition during the portion of the tests where we need to screenshot the closed sidebar. Then we re-enable the transition by removing the CSS that disabled it, before toggling the sidebar and screenshotting it in the open state.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cpsievert I've fixed the sidebar position, but it looks like theres a bug here that could be 1) a selectize issue; 2) an accordion issue; or 3) a bug in the test app. I poked at it a bit but didn't find an easy answer. (Note I also tried with the latest selectize fixes in shiny PR 3923.)

@gadenbuie gadenbuie changed the title fix: Disable transitions in sidebar tests with screenshots fix(306,308,310): Sidebar and accordion updates Oct 31, 2023
@gadenbuie gadenbuie merged commit a5a1bd9 into main Oct 31, 2023
4 of 19 checks passed
@gadenbuie gadenbuie deleted the sidebar/padding-transition branch October 31, 2023 03:04
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.

1 participant