-
Notifications
You must be signed in to change notification settings - Fork 0
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 table preview to data cards #97
Conversation
I am experiencing a red test:
|
As we cannot reproduce it with the CI job, my guess is that it is cache related. We handle the following cache folders:
But for audbcards/tests/test_datacard.py Line 131 in bb3820b
Can you maybe delete (or temporary move to another place) your |
I created #103 to ensure we always use the temporary cache folder when testing |
audbcards/core/utils.py
Outdated
@@ -99,6 +101,33 @@ def limit_presented_samples( | |||
return samples | |||
|
|||
|
|||
def parse_text(text: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether there is a reason to put this into the utils module? My understanding would be that this function has a small amout of usecases it can be reasonable applied to - but I could be wrong.
At least I can only image the current Dataset.tables_preview
right now. If I am right: would it not make sense to rather let it live in Dataset
as a static private method? For testing this would be not more problematic as staticmethods can be addressed without object instantiation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, testing was the only reason why added parse_text()
to utils
. I moved it now to a private static method of Dataset
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I had to develop such a feature myself, the only aspect I found off-putting would have been that the jinja becomes quite acrobatic.
I have tested this functionally:
I can recreate the documentation with the audeering theme and also alabaster.
Aesthetically this is a nice feature to have. In case of problems the preview will simply stop working, and the users will not be affected.
Therefore I will rush to approve it.
Closes #59
Adds a table preview to
audbcards
.audbcards.Dataset.tables_preview
The pull request adds the cached property
audbcards.Dataset.tables_preview
which returns a dictionary with table IDs as keys and a list of table rows as values.It downloads the whole table to create a preview. We might improve on this later on as described in #100.
Having #101 in mind, this pull request adds an usage example to the docstring of
audbcards.Dataset.tables_preview
and uses doctest to check its correctness.Integration in audbcards.Datacard
The pull request updates the
datacard_tables.j2
template to integrate the table preview in the existing tables table. The rows of the tables table are now clickable and show the preview table below the clicked row.Theme compatibility
When introducing new CSS based features, one has to ensure that they don't contain theme specific CSS code.
I tested it with the standard sphinx alabaster theme:
With inspid theme (requires another virtual environment with newer
sphinx
anddocutils
):With sphinx-audeering-theme:
Theme update
I further created already a pull request for
sphinx-audeering-theme
, that adds some custom styling to the new table preview (which was used in the animated gif above): audeering/sphinx-audeering-theme#84.Applying the update to the theme will result in
Ensure text in table preview is valid
As we include table content as string in an HTML table, not all characters are allowed. E.g. from our existing datasets
era-dialog
would result in an error, if the text is not adjusted. This pull request addsaudbcards.core.utils.parse_text()
to remove HTML characters, replace newline with"\\n"
and limit the length of the text to 100 characters.