Add a setting to more clearly enable scrolling on cell outputs #11222
Conversation
DavidKutu
requested review from
DonJayamanne,
greazer,
IanMatthewHuff,
jmew and
rchiodo
| @@ -536,9 +537,10 @@ export class CellOutput extends React.Component<ICellOutputProps> { | |||
| const style: React.CSSProperties = {}; | |||
|
|
|||
| // Create a scrollbar style if necessary | |||
| if (transformedList.some((transformed) => transformed.output.renderWithScrollbars) && this.props.maxTextSize) { | |||
There was a problem hiding this comment.
maxTextSize [](start = 105, length = 11)
We already has this value. Why is this not sufficient? It does the same thing
There was a problem hiding this comment.
@jmew and I thought renaming it to "Enable scrolling for cell outputs" and having it be a checkbox would be more clear to users (that's how the setting is named in jupyter lab). And I didn't want to remove the functionality to be able to resize the output so I left it in. I'm open to suggestions.
There was a problem hiding this comment.
But if maxOutputSize is set to -1, we will suddenly force everybody to have a max output size of 400. When they really wanted no scroll bars.
In reply to: 409903189 [](ancestors = 409903189)
There was a problem hiding this comment.
That never happens, if maxOutputSize is set to -1, weather this new setting is true or false, no scrollbar appears.
There was a problem hiding this comment.
Not sure I'm following the logic for that.
If maxOutputSize is set to -1, this.props.maxTextSize is undefined.
If the setting for enable scrolling is true, it will go to line 541, where suddenly the text size will be 400
In reply to: 409912783 [](ancestors = 409912783)
There was a problem hiding this comment.
and @IanMatthewHuff I dont believe they have a setting to control the max height for truncation
There was a problem hiding this comment.
I'm fine with the new setting, but we should fix the problem of it auto truncating when maxTextSize is -1. That would be a bug
In reply to: 409925767 [](ancestors = 409925767)
There was a problem hiding this comment.
maxTextSize of -1 should be equivalent to enableScrolling being false.
In reply to: 409930365 [](ancestors = 409930365,409925767)
There was a problem hiding this comment.
Cause otherwise you're going to break existing users.
In reply to: 409930466 [](ancestors = 409930466,409930365,409925767)
Codecov Report
@@ Coverage Diff @@
## master #11222 +/- ##
==========================================
- Coverage 61.39% 61.10% -0.30%
==========================================
Files 604 602 -2
Lines 33501 33146 -355
Branches 4739 4684 -55
==========================================
- Hits 20568 20253 -315
+ Misses 11877 11870 -7
+ Partials 1056 1023 -33
Continue to review full report at Codecov.
|
| @@ -536,9 +537,10 @@ export class CellOutput extends React.Component<ICellOutputProps> { | |||
| const style: React.CSSProperties = {}; | |||
|
|
|||
| // Create a scrollbar style if necessary | |||
| if (transformedList.some((transformed) => transformed.output.renderWithScrollbars) && this.props.maxTextSize) { | |||
| if (transformedList.some((transformed) => transformed.output.renderWithScrollbars) && this.props.enableScroll) { | |||
There was a problem hiding this comment.
Ideally we should just use react context. This way we don't have to pass down props through all these layers.. I know i've brought this up in the past... We keep modifying 3-4 files for such things.
the scrollbar
| const maxOutputSize = this.props.settings.maxOutputSize; | ||
| const maxTextSize = maxOutputSize && maxOutputSize < 10000 && maxOutputSize > 0 ? maxOutputSize : undefined; |
There was a problem hiding this comment.
This number 10000 is hardcoded in two places, time for a constant.
Also, I'd move the following code into its own function (this change is optional).
| const maxOutputSize = this.props.settings.maxOutputSize; | |
| const maxTextSize = maxOutputSize && maxOutputSize < 10000 && maxOutputSize > 0 ? maxOutputSize : undefined; | |
| const maxOutputSize = this.props.settings.maxOutputSize; | |
| const maxTextSize = maxOutputSize && maxOutputSize < 10000 && maxOutputSize > 0 ? maxOutputSize : undefined; |
There was a problem hiding this comment.
Still not done, number is hardcoded and identical code in two places.
Can just move into a function and re-use from two places.
Either use a constant for 1000 or add comments. Right now looking at the code I don't know what 10000, is it lines or character count, or pixels? (hence the suggestion to use a costant)
There was a problem hiding this comment.
By two places do you mean nativeEditor.tsx and interactivePanel.tsx? To reuse it in those 2 places I'd have to create a helpers file just for that one line function, or put it in another file that doesn't make sense. Is that what you want?
For now I used a constant
|
Kudos, SonarCloud Quality Gate passed!
|
For #9801
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).