Skip to content

Data Explorer Accessiblity#6704

Merged
IanMatthewHuff merged 46 commits into
microsoft:masterfrom
IanMatthewHuff:dev/ianhu/dataExplorerAccess3
Jul 25, 2019
Merged

Data Explorer Accessiblity#6704
IanMatthewHuff merged 46 commits into
microsoft:masterfrom
IanMatthewHuff:dev/ianhu/dataExplorerAccess3

Conversation

@IanMatthewHuff
Copy link
Copy Markdown
Member

For #6019

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Appropriate comments and documentation strings in the code
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated
  • Test plan is updated as appropriate
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)
  • The wiki is updated with any design decisions/details.

@IanMatthewHuff
Copy link
Copy Markdown
Member Author

Sorry phantom commits are back for some reason

Comment thread package-lock.json
}
},
"@types/jquery": {
"version": "3.3.29",
Copy link
Copy Markdown

@rchiodo rchiodo Jul 25, 2019

Choose a reason for hiding this comment

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

29 [](start = 22, length = 2)

This seems odd? Why did it go backwards? #ByDesign

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah, we didn't used to install it. Maybe it should be the newer version?


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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So what I did here was that the types were missing from the slick grid reference. So I added in the closest version to the version that slick grid was using 1.11.2 that's the version in slickgrid and the version that I use manually in reactSlickGrid.tsx. But I'm not sure that actually matters here, I think the newer ones would work as well?


In reply to: 307522767 [](ancestors = 307522767,307522512)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe not. They may not line up. Not sure how we make the person editing slickgrid realize this.


In reply to: 307525920 [](ancestors = 307525920,307522767,307522512)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not fully sure about this, so I can change if you think that I'm off. But I think that sticking with the type for the version of jquery that we are actually using (I think this is the only place we are actually using it in the extension) would be the right call for now at least.


In reply to: 307526879 [](ancestors = 307526879,307525920,307522767,307522512)

// Don't let the parent / browser do stuff if we handle it
// otherwise we'll both move the cell selection and scroll the window
// with up and down keys
e.stopPropagation();
Copy link
Copy Markdown

@rchiodo rchiodo Jul 25, 2019

Choose a reason for hiding this comment

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

stopPropagation [](start = 14, length = 15)

Does NVDA (or whatever the screen reader was called) still work? Thought you said this was messing it up? Or is it only if it swallows tab? #ByDesign

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, seemed ok to me. I wasn't getting the issues. I believe that it was StopImmediatePropagation that causes the issues. StopPropagation stops propagation for the current event. StopImmediatePropatagation prevents other listeners of the same event from being called.


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

// The slickgrid version of jquery populates keyCode not code, so use the numerical values here
switch (e.keyCode) {
// LeftArrow
case 37:
Copy link
Copy Markdown

@rchiodo rchiodo Jul 25, 2019

Choose a reason for hiding this comment

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

37 [](start = 21, length = 2)

I believe there's a constant for this somewhere.

Oh it's in the monacoEditor namespace. Might be a little weird to reference it here. #Resolved

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't want to use the monaco one, but even though I don't see reusing this much (we only use the keyCodes here since this jquery version doesn't populate code) this probably would look cleaner with a constant. I'll add one in like react-common or someplace like that.


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

Copy link
Copy Markdown

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

:shipit:

@IanMatthewHuff IanMatthewHuff merged commit 640a62b into microsoft:master Jul 25, 2019
@IanMatthewHuff IanMatthewHuff deleted the dev/ianhu/dataExplorerAccess3 branch July 25, 2019 23:49
@lock lock Bot locked as resolved and limited conversation to collaborators Aug 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants