Skip to content

Conversation

@nstrayer
Copy link
Collaborator

@nstrayer nstrayer commented Jul 7, 2023

The commits in this PR are all comments around a component's typescript implementation and python wrapper. Each commit corresponds roughly to an area/ theme and may have more than one component in it, however there's typically one "main" component and the others are all complementary ones (e.g. headers and footers for cards).

@jcheng5
Copy link

jcheng5 commented Jul 12, 2023

I lost almost all of Monday to meetings, and all of Tuesday to illness, but I'll try to do more review of this today. In the meantime, here are the few questions/comments that have come up so far:

  1. h3-5 look unstyled (except margin shifting)
  2. Are sidebars not allowed inside tabs? (page_dashboard > tab > sidebar) Currently it does not appear. If sidebars are only allowed in certain containers, let's validate and tell the app author, either in Python/R or in JS. Or have the API reflect that.
  3. If I'm using sc.tab inside of sc.page_dashboard, where do I put the dashboard title? I feel like I saw a title + tabs in one of your demos. (Answer: It's before_navigation, I just saw that parameter name and assumed it had to do with modifying navigation.) (Consider whether page_dashboard is doing too much for one function)
  4. I tried putting a ui.output_plot inside an sc.card and it didn't work; the height goes to zero. Setting an explicit height didn't fix it. (Edit: sc.forge.output_plot worked) (Edit: shinywidgets.output_widget did appear but seems to be hard coded to 400px height--setting an explicit height in the output_widget call does nothing. This might be unrelated to sc though)

@jcheng5
Copy link

jcheng5 commented Jul 12, 2023

  1. When sc.forge.output_plot is on a tab that's not the default, an error flashes when it's made visible (followed by the correct plot) (cc @wch)
  2. The Seaborn and Matplotlib plots I'm making have a white background by default, it contrasts with the interior of the card.
  3. General design question: were you envisioning that users would put basically all controls/outputs inside of a card/cards? (As opposed to directly inside of a sc.tab() or even sc.page_dashboard())
    7a. How do we balance conceptually clear building blocks with making the code accessible to people who don't know Shiny? (One way is to have high- and low-level functions for the different audiences?)

@jcheng5
Copy link

jcheng5 commented Jul 13, 2023

  1. A ui.input_slider inside of a card acts flexy.
image

@jcheng5
Copy link

jcheng5 commented Jul 13, 2023

  1. There are three places in the code where we assign the slot attribute on an argument that happens to be a Tag. It'd be better not to mutate the original object, but a copy.
  2. Would it make sense to have subtypes for some custom tags? Like sc.sidebar() -> SidebarTag, where SidebarTag = typing.NewType("SidebarTag", Tag). This would allow type annotations to call out tags that will be specially handled; for example, sc.page_dashboard's *args type could be TagChild | TagAttrs | SidebarTag | TabTag.

@nstrayer
Copy link
Collaborator Author

nstrayer commented Jul 20, 2023

1

h3-5 look unstyled (except margin shifting)

This was a bone-headed typo. I had too many closing parentheses for the variable declaration, and this caused the variable to be invalid and thus the font-size defaulted to 1rem. I have rainbow parens enabled and i thought red was just one of the colors. oops. (40c8521)

2

Are sidebars not allowed inside tabs? (page_dashboard > tab > sidebar) Currently it does not appear. If sidebars are only allowed in certain containers, let's validate and tell the app author, either in Python/R or in JS. Or have the API reflect that.

#8

3

If I'm using sc.tab inside of sc.page_dashboard, where do I put the dashboard title? I feel like I saw a title + tabs in one of your demos. (Answer: It's before_navigation, I just saw that parameter name and assumed it had to do with modifying navigation.) (Consider whether page_dashboard is doing too much for one function)

#9

4

I tried putting a ui.output_plot inside an sc.card and it didn't work; the height goes to zero. Setting an explicit height didn't fix it.

#10

5

When sc.forge.output_plot is on a tab that's not the default, an error flashes when it's made visible (followed by the correct plot) (cc @wch)

Coming back to this one

6

The Seaborn and Matplotlib plots I'm making have a white background by default, it contrasts with the interior of the card.

I wonder if a box-shadow or some other decoration could help this a bit. Either that or we push it to the edges.

7

General design question: were you envisioning that users would put basically all controls/outputs inside of a card/cards? (As opposed to directly inside of a sc.tab() or even sc.page_dashboard())
7a. How do we balance conceptually clear building blocks with making the code accessible to people who don't know Shiny? (One way is to have high- and low-level functions for the different audiences?)

I was effectively envisioning this, yeah. A card or div-like container. Given we're building apps that fill the page and put stuff in two dimensions, I think it's a reasonable expectation. Potentially tools like the UI editor/ copilot could help with beginning users who just want it to work. Ideally, we can also iterate on the API to add cleaner paths to more common use-cases when they become clear based on real-world usage.

8

A ui.input_slider inside of a card acts flexy.

Ah my old enemy, input_slider(). I was using a super-broad selector of div and sections being made to flex. Added :not(.shiny-input-container) to this to fix (8d02e74). We only really tested with our own custom input components so I never noticed this. (Hopefully this isn't a slippery slope of selector addition).

9

There are three places in the code where we assign the slot attribute on an argument that happens to be a Tag. It'd be better not to mutate the original object, but a copy.

#11

10

Would it make sense to have subtypes for some custom tags? Like sc.sidebar() -> SidebarTag, where SidebarTag = typing.NewType("SidebarTag", Tag). This would allow type annotations to call out tags that will be specially handled; for example, sc.page_dashboard's *args type could be TagChild | TagAttrs | SidebarTag | TabTag.

Yes. #12.
This also could help with 7, in that we could only have the args for say the dashboard compnent as *arg: CardTag | SidebarTag| ...

@jcheng5
Copy link

jcheng5 commented Aug 1, 2023

  1. ✅, that works great!
  2. (separate issue, listed here for completeness)
  3. Do you want me to file a separate issue for this?
  4. A border might look a bit "hat on a hat" if these are already inside of cards. If we push it to the edge of the card, it might look weird that just plot cards have a white background. What if either our default theme has white card backgrounds by default, or, we auto-match the plot to its container's colors. (Same problem with plotly's ipywidget-based plots, btw.)
  5. Upon reflection, I think this totally makes sense when starting with page_dashboard(). Whereas page_article you might not want plots to always be in cards, but you might also have the body bg color be white instead of the dashboard's darker shade.
  6. ✅ Felt great to click through on sc.tab function name and getting that hint on the *args.

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.

3 participants