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

Preserve white space in text plugin. #6833

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

arcra
Copy link
Member

@arcra arcra commented Apr 19, 2024

Some users require the spaces to be preserved in the text shown in the text dashboard.

At the moment I can't think of a reason why not to enable this unconditionally. I imagine anybody interested in seeing text logged would be interested in the value verbatim, including spaces.

Googlers, see b/335770352

@arcra arcra assigned bmd3k and unassigned bmd3k Apr 19, 2024
@arcra arcra requested a review from bmd3k April 19, 2024 05:45
@@ -46,6 +46,9 @@ class TfMarkdownView extends LegacyElementMixin(PolymerElement) {
#markdown > p:last-child {
margin-bottom: 0.3em;
}
#markdown p {
white-space: pre;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will have the unintended consequence of meaning that actual paragraphs of text will not be wrapped in a sane manner.

For the use case you are investigating, I think we should understand why the markdown renderer is wrapping some text with <pre><code></code></pre> and other code with just <p>.

I suspect it has to do with the non alpha-numeric text included in the results but that is just speculation.

Markdown Rendering is here:
https://github.com/tensorflow/tensorboard/blob/master/tensorboard/plugin_util.py

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Brian, that's a good point. I'll look into that thing from the markdown converter until, but also, how about using white-space: break-spaces; instead? Looks like this pretty much solves the problem by allowing text to wrap, but preserving spaces: https://developer.mozilla.org/en-US/docs/Web/CSS/white-space

Copy link
Member Author

@arcra arcra Apr 19, 2024

Choose a reason for hiding this comment

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

That result with the <pre> tags seems to come from the markdown library. I don't think it's worth trying to figure out how that works. I'm more inclined to just go with the white-space: break-spaces; solution. If users have an issue with this, we can add a checkbox control later, similar to the "markdown" one, tho I imagine most users would want to see the spaces if they're there.

@@ -46,6 +46,9 @@ class TfMarkdownView extends LegacyElementMixin(PolymerElement) {
#markdown > p:last-child {
margin-bottom: 0.3em;
}
#markdown p {
white-space: break-spaces;
Copy link
Contributor

@bmd3k bmd3k Apr 19, 2024

Choose a reason for hiding this comment

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

Mind adding a comment or just a link to the bug (imo its ok that the bug is internal only)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@arcra arcra merged commit f2e8bd8 into tensorflow:master Apr 19, 2024
13 checks passed
@arcra arcra deleted the text_plugin_preserve_whitespace branch April 19, 2024 21:31
AnuarTB pushed a commit to AnuarTB/tensorboard that referenced this pull request May 3, 2024
Some users require the spaces to be preserved in the text shown in the
text dashboard.

At the moment I can't think of a reason why not to enable this
unconditionally. I imagine anybody interested in seeing text logged
would be interested in the value *verbatim*, including spaces.

Googlers, see b/335770352
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.

None yet

2 participants