Skip to content

More realistic file import example #582

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Aug 14, 2023
Merged

More realistic file import example #582

merged 17 commits into from
Aug 14, 2023

Conversation

adejumoridwan
Copy link
Contributor

@adejumoridwan adejumoridwan commented Jun 23, 2023 •
edited
Loading

Closes #567
Change the file import example to display summary stats of uploaded data, such as row count, column count and column names.

@CLAassistant
Copy link

CLAassistant commented Jun 23, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@adejumoridwan adejumoridwan changed the title More realistic file import example #567 More realistic file import example Jun 23, 2023
@gshotwell gshotwell modified the milestone: sprint-august-1 Aug 2, 2023
@gshotwell gshotwell requested a review from wch August 8, 2023 12:17
@wch
Copy link
Collaborator

wch commented Aug 8, 2023

This PR includes your Snowflake credentials. That doesn't seem right...

We appreciate this PR, but it's not clear to me that it is an improvement for the intended purpose of the app, which is just to illustrate how to use the file upload component. The old app just shows how to do file uploads, while the new one also adds some interactivity with checkboxes. The new version is in a sense a more realistic app, but in this case I don't think the changes are helpful for illustrating how the file upload component works.

@wch
Copy link
Collaborator

wch commented Aug 8, 2023

Apologies, I looked at this too quickly and didn't see the linked issue at the top, and the discussion in that issue.

@wch wch reopened this Aug 8, 2023
@adejumoridwan
Copy link
Contributor Author

No problem, as for the snowflake credentials, it has been removed

@wch
Copy link
Collaborator

wch commented Aug 8, 2023

Great, thanks!

Just a heads up: I'm working on some review comments now but I have a meeting so it'll be a couple hours before I'll be able to submit them.

@gshotwell
Copy link
Contributor

Thanks, I think I should've chimed in a bit more since I think I suggested this PR. The main things I find confusing about the current example are:

  • It uses conditional UI which people might not know (people probably build file-import apps before they build conditional UI apps)
  • It uses ui.HTML output to render a table which isn't that common (most of the time people use ui.output_table or ui.output_data_frame

wch
wch requested changes Aug 8, 2023
View reviewed changes
Copy link
Collaborator

@wch wch left a comment

Choose a reason for hiding this comment

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

I've added some comments which apply to both copies of the app. Thanks for your efforts!

shiny/examples/input_file/app.py Outdated
app_ui = ui.page_fluid(
ui.input_file("file", "File", accept=".csv"),
ui.input_checkbox_group(
"stats", "Summary Stats", choices=["Row Count", "Column Count", "Column Names"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of the checkboxes start unselected, so if someone uploads a file without toggling the checkboxes, they won't see any output. Please add selected=["Row Count", "Column Count", "Column Names"] so that they will see the table by default.

@adejumoridwan adejumoridwan requested a review from wch August 8, 2023 20:22
wch
wch approved these changes Aug 14, 2023
View reviewed changes
Copy link
Collaborator

@wch wch left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good - I found that the two versions of app.py aren't quite the same so I'll get them in sync and merge.

Update: It looks like I'll have to merge and then fix in a separate commit.

@wch wch merged commit db19821 into posit-dev:main Aug 14, 2023
schloerke added a commit that referenced this pull request Aug 22, 2023
@schloerke
Merge branch 'main' into pop_over
57fae0b 
* main:
  feat(Session): Make Session on_flush() and on_flushed() accept async functions (#693)
  Make data frame selection return row numbers, not pandas index value (#677)
  chore(api)!: Rename `ui.navset_pill_card` -> `ui.navset_card_pill` and `ui.navset_tab_card` -> `ui.navset_card_tab` (#681)
  Consolidate all testing into `tests/` folder (#683)
  Fix pyright error (#678)
  Make model score app work on Connect/Shinyapps.io (#657)
  Suppress type check for read_csv
  Synchonize input_file examples
  More realistic file import example (#582)
  Make flaky dataframe test have larger timeout (#675)
  Wrap bare value box value in `p` tag (#668)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wch wch wch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

More realistic file import example
4 participants
@adejumoridwan @CLAassistant @wch @gshotwell