Initial set of Keytips work in experiments by kelseyyoung · Pull Request #4062 · microsoft/fluentui

kelseyyoung · 2018-02-21T22:21:22Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change

Description of changes

Experiments: Initial set of work for Keytips. See Keytip and KeytipLayer demo pages in experiments

Utilities: Add missing key codes to KeyCodes enum

Focus areas to test

- Made key sequences in KeytipLayer optional again - Add utility constants, refactored code - Moved IKeySequence to string code from KeytipManager to IKeytipSequence, also moved tests

- Manually add Keytip through manager after mount for experiments

…ests

…fice-ui-fabric-react into features/keytips2

- Keytips will now hide on scroll

- Introduce idea of modifier key codes - Split processInput with processTransitionInput

- Remove ID and KeytipTarget from Keytip props

- Added some documentation and Dos/Donts

…fice-ui-fabric-react into features/keytips2

- Move and rename ModifierKeyCodes to Keytip.types to prevent errors in testing

christiango · 2018-02-22T17:31:34Z

packages/experiments/src/components/KeytipLayer/KeytipLayer.tsx

+      // Try to find keytipProps in previousKeytips to update
+      let keytipToUpdateIndex = -1;
+      for (let i = 0; i < previousKeytips.length; i++) {
+        if (fullKeySequencesAreEqual(keytipProps.keySequences, previousKeytips[i].keySequences)) {


Use findIndex from the utilities package here instead of rolling your own loop.

christiango · 2018-02-22T17:37:30Z

packages/experiments/src/utilities/keysequence/IKeySequence.ts

+ * @returns {boolean} T/F if 'sequences' contains 'seq'
+ */
+export function keySequencesContain(sequences: IKeySequence[], seq: IKeySequence): boolean {
+  for (let i = 0; i < sequences.length; i++) {


Seems like this is just an implemetation of includes, unfortunately this doesn't work in IE11, but we can add an implementation in utilities (like there is for findIndex) to use to reduce boilerplate here in a few places. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes

@christiango can we use includes() if we have an array of objects? (in this case, IKeySequence[]) I would think then we would need an object deep equals function...

at its base IKeySequence.keys is an array of strings, but that is an equality check we do with .join(), it wouldn't be an includes() check there

You can use find if you need more elaborate comparison logic

christiango · 2018-02-22T17:39:14Z

packages/experiments/src/utilities/keysequence/IKeySequence.ts

+ */
+export function convertSequencesToKeytipID(keySequences: IKeySequence[]): string {
+  let conversion = ktpPrefix;
+  for (let keySequence of keySequences) {


Seems like a good candidate for array.prototype.reduce instead of a for of loop

christiango · 2018-02-22T17:42:33Z

packages/experiments/src/utilities/keysequence/IKeytipTransitionKey.ts

+    if (mod2Copy.length !== 0) {
+      return false;
+    }
+  }


This is N^2, can we use a JavaScript set instead? I'm not sure what the policy on using Set is in fabric, it is supported by IE11, but it is in typescripts ES2015.collections library, which is not included by default when transpiling to ES5. Thoughts @dzearing ?

What do you think about sorting the two arrays and looping through? The arrays are just of enums (so numbers) so they can be sorted naturally

How big do you expect these arrays to get? Sorting will still be slower than using the set, which might matter if these lists get pretty long or this code runs a lot.

The arrays are just a list of modifiers (Alt, Shift, Ctrl). We only have 5 defined now so that would be the max, but the developer wouldn't define more then 2-3 (otherwise the user has to press 5 keys to enable keytip mode)

Sorting is fine then

christiango · 2018-02-22T17:43:35Z

packages/experiments/src/utilities/keysequence/IKeytipTransitionKey.ts

+ * @returns {boolean} T/F if 'keys' contains 'key'
+ */
+export function transitionKeysContain(keys: IKeytipTransitionKey[], key: IKeytipTransitionKey): boolean {
+  for (let i = 0; i < keys.length; i++) {


In general, go through all the for loops we have here and see if we can use some of the built in higher order functions in JavaScript (or the equivalent implementation in fabric utilities when that API is not supported in IE11)

Yeah there were a few places where we wanted to use those functions but didn't because of compatibility. I didn't know there would be equivalent ones in fabric so thanks for pointing that out!

- Use reduce or map where applicable

…fice-ui-fabric-react into features/keytips2

dzearing · 2018-03-01T21:46:03Z

packages/experiments/src/components/KeytipLayer/examples/KeytipLayer.Basic.Example.tsx

+          <div style={ divStyle }>
+            <div>
+              <ActionButton
+                data-ktp-id={ convertSequencesToKeytipID(this.keytipMap.Pivot1Keytip.keySequences) }


feedback (not required to change in this PR)

abstract in a helper function so that caller doesn't know about data-*.

Also this is quite verbose. There needs to be a better experience than to call an API convertSequencesToKeytipID here for the caller.

If we go with the helper, maybe BaseComponent could auto dispose the cached options from global state, if that's what we do to get access to the global state.

private _keytips = this.autoDispose(registerKeytips({ pivot1: { options }, pivot2: { options } })); // ...and then: <ActionButton { ...this._keytips.pivot1 } />

Right, this is only because the keytipProps haven't been added to ActionButton. We have to mock it right now because we haven't moved to office-ui-fabric-react. Hardcoding the data-ktp-id would be replaced with just defining keytipProps

dzearing · 2018-03-01T23:23:39Z

packages/experiments/src/components/KeytipLayer/examples/KeytipLayer.Basic.Example.tsx

+    // Manually add keytips to the KeytipManager for now
+    // This should really be done in each component
+    for (let component of Object.keys(this.keytipMap)) {
+      registerKeytip(this.keytipMap[component]);


I see a registerKeytip call but no unregisterKeytip.

I am really nervous about this pattern because if it's easy for you to forget to do this, it will be easy for partners to as well.

Yes, this is also mocked, the register and unregister should only be in the component render() and componentWillUnmount() (I know we have discussed this, we can try to come up with a better solution). In this sample we don't have any dynamic DOM elements so we aren't doing any unregistering. When we move to the normal package we will have keytips on menus, etc

isn't the sample itself dynamic? E.g. you will have a memory leak if you go to the page, then to another, then back to the original.

dzearing · 2018-03-01T23:24:33Z

packages/experiments/src/components/KeytipLayer/KeytipLayer.tsx

+   *
+   * @param keytipProps - IKeytipProps to add to this layer
+   */
+  public registerKeytip(keytipProps: IKeytipProps): void {


Rather than an API which takes a single keytip, why not take in a map of keytips?

register should only be called from one component''s render() at a time, so I don't see a use case for registering multiple right now but if we come across one we can definitely do it! would be very easy

dzearing

Added some feedback! But let's get it in and iterate.

* master: (30 commits) Addressing Issue microsoft#4147 - Nav Buttons have invalid `aria-describedby` value (microsoft#4159) Fix issue 3608: DetailsList horizontal scroll (microsoft#4164) Update package.json Image SCSS to MergeStyles Part 2: Style Conversion (microsoft#4151) Applying package updates. [SpinButton] Consistent styles to Button and TextField (microsoft#4098) ChoiceGroup: Flex layout for image and icon options (microsoft#4137) Initial set of Keytips work in experiments (microsoft#4062) Updating tsconfig in button bundle. BaseExtendedPicker: Create contextmenu for renderedItem, fix auto focus (microsoft#3954) Dropdown: Custom render options for multiselect - Bug fix microsoft#3571 (microsoft#3589) Fix documentcard theming (microsoft#4155) BasePicker: Fix not used onBlur callback of inputProps (microsoft#4000) (microsoft#4131) Allow Elements as Callout targets (microsoft#4134) CommandBar: Fixed null commandItemWidths (microsoft#4136) add check for when this.suggestionElement may be undefined (microsoft#4157) Addressing Issue microsoft#4143 - is-selected missing on ms-Nav-link (microsoft#4158) Aria selected (microsoft#4161) Move fabric to TypeScript 2.7.2 (microsoft#4153) Updating SearchBox examples to have the removed string in placeholder prop. ...

* master: (51 commits) Applying package updates. No unused variable (microsoft#4173) Use correct _list ref string (microsoft#4168) Nav: wire a link to expand/collapse behavior if it has no URL but has children (microsoft#4171) Addressing Issue microsoft#4147 - Nav Buttons have invalid `aria-describedby` value (microsoft#4159) Fix issue 3608: DetailsList horizontal scroll (microsoft#4164) Update package.json Image SCSS to MergeStyles Part 2: Style Conversion (microsoft#4151) Applying package updates. [SpinButton] Consistent styles to Button and TextField (microsoft#4098) ChoiceGroup: Flex layout for image and icon options (microsoft#4137) Initial set of Keytips work in experiments (microsoft#4062) Updating tsconfig in button bundle. BaseExtendedPicker: Create contextmenu for renderedItem, fix auto focus (microsoft#3954) Dropdown: Custom render options for multiselect - Bug fix microsoft#3571 (microsoft#3589) Fix documentcard theming (microsoft#4155) BasePicker: Fix not used onBlur callback of inputProps (microsoft#4000) (microsoft#4131) Allow Elements as Callout targets (microsoft#4134) CommandBar: Fixed null commandItemWidths (microsoft#4136) add check for when this.suggestionElement may be undefined (microsoft#4157) ...

kelseyyoung and others added 30 commits February 1, 2018 16:50

Adding files to experiments

2f24d99

Fix most tslint errors

76352b0

Fix initial tslint errors

3543cc3

Fix other tslint errors

67627a4

Fix last compiling issues

cbd290b

Add keysequence files

51cc7ef

Add pages to example app

2e2af82

Removed IKeySequence files in packages/utilities

2af68af

- Made key sequences in KeytipLayer optional again - Add utility constants, refactored code - Moved IKeySequence to string code from KeytipManager to IKeytipSequence, also moved tests

First pass of writing processInput

0a062cd

Update KeytipLayer Example to use data attribute

36fd2eb

- Manually add Keytip through manager after mount for experiments

Add Keytipsequencestarts with tests

92fbf3a

Add matched and partially matched nodes tests

fdf62fe

Add keysequenceStarts with tests and partial and exact node matched t…

c4ad656

…ests

Add more tests to keysquences

d8df180

Add initial processInput tests

2c2b93c

Add initial processInput tests

4cffb82

Clean up unused imports

186a0c6

Merge branch 'features/keytips2' of https://github.com/kelseyyoung/of…

99a5f34

…fice-ui-fabric-react into features/keytips2

Add partial nodes tests

361a486

Show/hide methods for the tree nodes

31d12d1

Add hasChildrenNodes prop and a test for it

c400a87

Moved function to comply with tslint

bd1bceb

Merge with features/keytips2

e0376be

Get keytip to show on starting sequence in example

780e149

- Keytips will now hide on scroll

Refactor KeySequences to use strings instead of KeyCodes

3f410d5

- Introduce idea of modifier key codes - Split processInput with processTransitionInput

disable and enable keytipmode correctly

e7c4aff

Add more items to KeytipLayer example

6f5ab2a

- Remove ID and KeytipTarget from Keytip props

Fix unit tests

4d90cb8

Fix exit points for keys

49aca3b

Exit when scrolled and enter keytipmode again

67805a0

kelseyyoung and others added 14 commits February 20, 2018 13:57

Comments and small refactoring

34f92f4

Add refactoring changes for ketip and keytipcontent

2fdc1a2

Add tests

ffec17f

Very basic layer test

8106e73

Added examples for Keytip page

38e04e6

- Added some documentation and Dos/Donts

Merge with features/keytips2

cd531ac

Remove unused keytipstate

eaaba1c

Remove unused files

dc82938

Small fixes

f594b05

Merge branch 'features/keytips2' of https://github.com/kelseyyoung/of…

a6140d4

…fice-ui-fabric-react into features/keytips2

Fix Keytip language example

c71a890

Merge remote-tracking branch 'upstream/master' into features/keytips2

5cac5fa

Make KeyCodes a const enum again

74cf1d4

- Move and rename ModifierKeyCodes to Keytip.types to prevent errors in testing

Add rush change files

ea81f58

montselozanod requested review from christiango, dzearing and jspurlin February 21, 2018 23:57

montselozanod added the Area: Accessibility label Feb 22, 2018

christiango reviewed Feb 22, 2018

View reviewed changes

montselozanod and others added 3 commits February 22, 2018 13:41

Add offset default prop and test UI for offset

17f122d

Improve code with Fabric array utils

59baba9

- Use reduce or map where applicable

Merge branch 'features/keytips2' of https://github.com/kelseyyoung/of…

ba40a98

…fice-ui-fabric-react into features/keytips2

dzearing reviewed Mar 1, 2018

View reviewed changes

dzearing approved these changes Mar 1, 2018

View reviewed changes

dzearing merged commit 44ca7b8 into microsoft:master Mar 2, 2018

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Conversation

kelseyyoung commented Feb 21, 2018 • edited by montselozanod Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Focus areas to test

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelseyyoung Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzearing Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dzearing left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kelseyyoung commented Feb 21, 2018 •

edited by montselozanod

Loading

kelseyyoung Feb 22, 2018 •

edited

Loading

dzearing Mar 1, 2018 •

edited

Loading