When pressing ctrl+enter on a markdown cell, unfocus it so it renders #11162

DavidKutu · 2020-04-14T21:36:32Z

For #10006

unfocus it so it renders

rchiodo · 2020-04-14T22:00:10Z

src/datascience-ui/native-editor/nativeCell.tsx

@@ -448,6 +448,11 @@ export class NativeCell extends React.Component<INativeCellProps> {
        e.stopPropagation();
        e.preventDefault();

+        // Unfocus the current cell if it is markdown to make it render
+        if (this.isMarkdownCell() && this.wrapperRef && this.wrapperRef.current && this.isFocused()) {
+            this.wrapperRef.current.focus();


this should probably use the this.props.unfocusCell instead. Although it would have the same net effect.

There's an issue with doing that, by focusing again on the same cell we still are in command mode. If I do the unfocusCell it unfocuses a lot and I can't use the commands anymore, I'd have to click on the notebook again.

Maybe I could change the comment to say 'unfocus and focus back'?

Not sure I understand this part:

If I do the unfocusCell it unfocuses a lot

Shouldn't it just unfocus this one cell? What do you mean by it unfocuses a lot?

In reply to: 408468988 [](ancestors = 408468988)

Are you saying the notebook itself no longer has focus?

In reply to: 408469794 [](ancestors = 408469794,408468988)

yes, pressing up or down no longer works, you need to click the notebook

btw, focusing in the wrapper is the same behavior as pressing escape. I think that's good enough.

Escape handling does look similar to this. Honestly I find some of the focus handling a bit twisty myself. So after you do a ctrl+enter on a markdown cell our HTML focus is on the wrapping div, but the cell vm state is still both selected and focused, is that right?

The blue bar appears, I think so. I'll check.

Can confirm, its still selected and focused.

rchiodo

🕐

codecov-io · 2020-04-14T22:42:08Z

Codecov Report

Merging #11162 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11162      +/-   ##
==========================================
+ Coverage   61.41%   61.43%   +0.02%     
==========================================
  Files         596      596              
  Lines       32848    32874      +26     
  Branches     4655     4659       +4     
==========================================
+ Hits        20173    20196      +23     
+ Misses      12465    11659     -806     
- Partials      210     1019     +809

Impacted Files	Coverage Δ
src/datascience-ui/react-common/arePathsSame.ts	`75.00% <0.00%> (-12.50%)`	⬇️
src/client/common/utils/platform.ts	`64.70% <0.00%> (-11.77%)`	⬇️
src/client/linters/pydocstyle.ts	`86.66% <0.00%> (-2.23%)`	⬇️
src/client/datascience/debugLocationTracker.ts	`76.56% <0.00%> (-1.57%)`	⬇️
...client/datascience/kernel-launcher/kernelFinder.ts	`75.70% <0.00%> (-0.94%)`	⬇️
src/client/common/process/proc.ts	`14.49% <0.00%> (-0.73%)`	⬇️
...ient/interpreter/locators/services/condaService.ts	`85.55% <0.00%> (-0.08%)`	⬇️
src/client/datascience/webViewHost.ts	`9.31% <0.00%> (-0.06%)`	⬇️
src/client/testing/main.ts	`14.38% <0.00%> (ø)`
src/client/common/editor.ts	`8.25% <0.00%> (ø)`
... and 174 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eab579f...c7a16b8. Read the comment docs.

rchiodo

DonJayamanne

I'd like to have a look tomorrow. Not happy that we're setting focus/unfocus like this. We had a few issues in the past.
Not saying the fix is not correct
Blocking till I check it out, thanks and sorry

DonJayamanne · 2020-04-14T21:57:19Z

src/datascience-ui/native-editor/nativeCell.tsx

@@ -448,6 +448,11 @@ export class NativeCell extends React.Component<INativeCellProps> {
        e.stopPropagation();
        e.preventDefault();

+        // Unfocus the current cell if it is markdown to make it render
+        if (this.isMarkdownCell() && this.wrapperRef && this.wrapperRef.current && this.isFocused()) {
+            this.wrapperRef.current.focus();


Please could we try to use existing mechanisms to set focus to the editor.

DonJayamanne · 2020-04-15T17:09:00Z

src/datascience-ui/native-editor/nativeCell.tsx

@@ -448,6 +448,11 @@ export class NativeCell extends React.Component<INativeCellProps> {
        e.stopPropagation();
        e.preventDefault();

+        // Refocus the current cell if it is markdown to make it render
+        if (this.isMarkdownCell() && this.wrapperRef && this.wrapperRef.current && this.isFocused()) {
+            this.wrapperRef.current.focus();


Suggested change

this.wrapperRef.current.focus();

this.escapeCell(e);

Please can we use escapeCell, at the end of the day thats what is required. We need to escape the cell. Manually focussing does the exact same thing, but this way its obvious what we are doing and why. With the other approach its unclear. I.e. we know hitting escape will render the markdown, hence we're merely using existing behaviour.

Also, I would remove the comments as it isn't necessary, at least thats my opinion, cuz to me its a little confusing (cuz we want to focus the cell however isFocused is already true for the nativeCell - i.e. contradictory).

The comment now says: // Escape the current cell if it is markdown to make it render

sonarqubecloud · 2020-04-15T17:18:13Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

DavidKutu added 2 commits April 14, 2020 14:33

When pressing ctrl+enter on a markdown cell,

903a617

unfocus it so it renders

add news file

9c17197

DavidKutu requested review from DonJayamanne, IanMatthewHuff and rchiodo April 14, 2020 21:38

rchiodo reviewed Apr 14, 2020

View reviewed changes

rchiodo requested changes Apr 14, 2020

View reviewed changes

rchiodo approved these changes Apr 14, 2020

View reviewed changes

changed a comment

a558fef

DonJayamanne requested changes Apr 15, 2020

View reviewed changes

reuse the function to escape the cell

c7a16b8

DavidKutu requested a review from DonJayamanne April 15, 2020 17:32

DonJayamanne approved these changes Apr 15, 2020

View reviewed changes

DavidKutu merged commit 02ff345 into master Apr 15, 2020

DavidKutu deleted the davidkutu/latexNotRendering branch April 15, 2020 18:51

lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020

When pressing ctrl+enter on a markdown cell, unfocus it so it renders #11162

When pressing ctrl+enter on a markdown cell, unfocus it so it renders #11162

Uh oh!

Conversation

DavidKutu commented Apr 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

DonJayamanne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 15, 2020

Uh oh!

Uh oh!

codecov-io commented Apr 14, 2020 •

edited

Loading