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 document viewer to dataset explorer #666

Merged
merged 14 commits into from
Nov 23, 2023
Merged

Conversation

PhilippeMoussalli
Copy link
Contributor

PR that adds document viewer to data explorer. The viewer attempts to automatically detect the string type and render it to its most appropriate format. Note that the normal string value does not get rendered to a pdf value but rather a normal text/markdown.

I don't think it makes a lot of sense to render normal unformated text to pdf. We can still detect whether a strings is a base64 pdf encoded string and render that to it's original pdf format.

Screenshot from 2023-11-22 17-32-32

@PhilippeMoussalli PhilippeMoussalli changed the title add document viewer to dataset explorer Add document viewer to dataset explorer Nov 22, 2023
Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks @PhilippeMoussalli! Will this be the only way to view text that is too long to display, or will you still add a tooltip as well?

@PhilippeMoussalli
Copy link
Contributor Author

Thanks @PhilippeMoussalli! Will this be the only way to view text that is too long to display, or will you still add a tooltip as well?

I was trying to include it but did not manage to get it working (requires some custom java script). Someone posted a solution recently but It did not work on my end. I asked them for more info

https://discuss.streamlit.io/t/tooltip-with-aggrid/55396

Base automatically changed from restructure-data-explorer to main November 23, 2023 13:52
@RobbeSneyders
Copy link
Member

I was trying to include it but did not manage to get it working (requires some custom java script). Someone posted a solution recently but It did not work on my end. I asked them for more info

https://discuss.streamlit.io/t/tooltip-with-aggrid/55396

Just did a quick test, and this seems to work:

for field in fields:
    options_builder.configure_column(field=field, tooltipField=field)

It's quite slow to show up the first time though.

@RobbeSneyders
Copy link
Member

Seems like the delay might be configurable:
https://stackoverflow.com/questions/55805496/ag-grid-tooltip-takes-long-time-to-render

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

I think you accidentally removed the document viewer 😅



class DatasetExplorerApp(DatasetLoaderApp):
@staticmethod
def setup_app_page(dataframe, fields):
def _get_hover_vis_script(field: str) -> JsCode:
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to add this custom javascript? I think the original style fits better with the style of the data explorer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it, I thought having some contrast might be a bit nicer but we can keep it simple

@PhilippeMoussalli
Copy link
Contributor Author

I think you accidentally removed the document viewer 😅

woops, readded it!

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

@PhilippeMoussalli PhilippeMoussalli merged commit f9b4b81 into main Nov 23, 2023
6 checks passed
@PhilippeMoussalli PhilippeMoussalli deleted the enhance-document-view branch November 23, 2023 16:39
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.

2 participants