Add Keytips to office-ui-fabric-react by kelseyyoung · Pull Request #4629 · microsoft/fluentui

kelseyyoung · 2018-04-20T02:11:06Z

Pull request checklist

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

Description of changes

Screenshot of keytips

Focus areas to test

(optional)

- Add keytipProps to BaseButton - Update KeytipLayer example to use new keytipProps, comment out some of it

- Add KeytipConfig files, use for setup in KeytipLayer

- Setup offset for keytips correctly - Add keytip offset example

- Remove keytipLink and previous overflow logic, and tests

- Update tests - Add KeytipLayer styles to prioritize that layer above all others

- Add split button in contextual menu to example

…s-to-package

…flexibility - Add more components to the KeytipLayer example - Add ariaDescribedBy to KeytipHost to do automatic appending

- Also unsets when item gets moved out of the overflow - Add logic in places to merge the overflow sequence if necessary - Fix tests that used let instead of const - Add persisted attribute to tree node to filter out when showing keytips

…le function ordering - Fix broken Keytip examples

dzearing · 2018-05-01T05:47:24Z

-  pauseBreak = 19,
-  capslock = 20,
  escape = 27,
  space = 32,


What exactly is going on in this file?

dzearing · 2018-05-01T05:48:17Z

+
+// Events
+export namespace KeytipEvents {
+  export const KEYTIP_ADDED = 'keytipAdded';


can you move the keytips stuff out of the utilities package? This will be included in the PLT1 bundle. It should only be in the keytips project in oufr.

dzearing · 2018-05-01T05:48:59Z

 export * from './IStyleFunction';
 export * from './KeyCodes';
+export * from './KeytipConstants';
+export * from './KeytipTransitionModifier';


None of the keytip work belongs in @uifabric/utilities.

dzearing · 2018-05-01T05:49:34Z

  up: 38,
  right: 39,
  down: 40,
-  insert: 45,


not sure what's up here.

dzearing · 2018-05-01T05:51:05Z

+  /**
+   * The background color for a disabled keytip.
+   */
+  keytipBackgroundDisabled: string;


it is weird that there is a background for a disabled keytip but nothing else...

jspurlin · 2018-05-01T16:17:59Z

+            aria-haspopup={ true }
+            aria-expanded={ this._isExpanded }
+            aria-pressed={ this.props.checked }
+            aria-describedby={ ariaDescribedBy + (keytipAttributes['aria-describedby'] || '') }


are you guaranteed that ariaDescribedBy and the aria-describedby from keytipAttributes will have a space in between them? Consider something like ${ariaDescribedby} ${keytipAttributes['ariadescribedby']}

The beginning space is added in the getAriaDescribedBy function

jspurlin · 2018-05-01T17:04:18Z

+import { IStyleFunction, IPoint } from '../../Utilities';
+import { mergeStyleSets } from '../../Styling';

 export const getStyles = (props: IKeytipStyleProps): IKeytipStyles => {


I'm not seeing any HC styling for keytips, when disabled the color should be grayText and the opacity should be 1

jspurlin

a few minor comments

- Add HC styling for keytips when disabled - Restored missing KeyCodes

…s-to-package

phkuo · 2018-05-01T18:47:33Z

 $listItemBackgroundCheckedHoveredColor: "[theme:listItemBackgroundCheckedHovered, default: #d0d0d0]";
+
+/* Keytips */
+$keytipDisabledTextColor: "[theme:keytipDisabledText, default: #b1b1b1]";


keytips should not get their own semantic slot
semantic slots are not for each feature to have their own, but for each concept
can you reuse an existing concept?

@phkuo How can I reuse an existing one if the color isn't shared with anyone else? I thought your previous comments in the other PRs (the part 1 and 2 ones) said to follow this pattern

All colors should be one that's in our palette, we shouldn't be adding things with bespoke colors.
For now, just use one of the palette slots from here:
https://github.com/OfficeDev/office-ui-fabric-react/blob/master/packages/office-ui-fabric-react/src/common/_themeOverrides.scss

Now using neutralTertiaryAlt instead which is very close to our original color

phkuo

see comment

phkuo · 2018-05-01T19:12:42Z

Can you add a screenshot?

.

kelseyyoung · 2018-05-01T20:01:25Z

 * This category covers all kinds of lists, whether they're typical one-item-per-row lists (like DetailsList) or ones with a tiled layout.
+ *
+ * ## Keytips
+ *


Oversight, will remove

- Created arraysAreEqual utility function

cliffkoh · 2018-05-03T02:15:31Z

Hi @kelseyyoung, because the Command Bar component you modified was replaced by the version in the @uifabric/experiments folder in 6.0, the keytips support for command bar will be lost when 6.0 lands.

I've therefore marked this PR as requiring follow up in 6.0. I will coordinate with you offline on what this means.

(cc @dzearing)

cliffkoh · 2018-05-03T02:16:29Z

Also @micahgodbolt for FYI.

kelseyyoung added 30 commits March 2, 2018 11:02

Commit start of moving files

7404874

Modify keytip files to compile and run correctly

feefb12

Move some utils to office-ui-fabric-react/utilities

6f9e6fb

- Add keytipProps to BaseButton - Update KeytipLayer example to use new keytipProps, comment out some of it

Moving work for contextual menu

0938eb8

- Add KeytipConfig files, use for setup in KeytipLayer

Add word for keytip offset

48c77fb

Allow function to be defined for Callout target

4757717

- Setup offset for keytips correctly - Add keytip offset example

Add logic for overflow set keytip manipulation

447571a

- Remove keytipLink and previous overflow logic, and tests

Merge with upstream master

668a03e

Latest design for keytips

5c35836

Update to -latest- design

59d8706

Add KeytipHost to manage optional keytipProps

182768c

- Update tests - Add KeytipLayer styles to prioritize that layer above all others

Fix Keytip example pages

7eba09a

Add keytips to Fabric components and KeytipLayer demo

5e9023d

Add comma separator for aria-describedby for keytips

7792acf

Fix some TODOs

7af662d

Merge with upstream master

7dafbdc

Finish master merge

28e21c3

- Add split button in contextual menu to example

Merge remote-tracking branch 'upstream/master' into keyou/move-keytip…

7c5a269

…s-to-package

Add data-ktp-target and data-ktp-execute-target for better developer …

b0d0cbb

…flexibility - Add more components to the KeytipLayer example - Add ariaDescribedBy to KeytipHost to do automatic appending

Update overflow logic and add example in KeytipLayer example

71378ab

Progress on overflow set tests

dee6a9e

Comment out failing test

4ac637f

Fix overflow tests that weren't passing

bbe697a

Cleanup and refactoring, added some tests

e57d6a5

merge with upstream master

911407d

Delete keytip layer in experiments

fd9cde1

Fix BaseButton formatting

58e6ff6

Add a unique ID to each keytip instance to deal with React 16 lifecyc…

706033c

…le function ordering - Fix broken Keytip examples

Separate KeytipTree add and update

eb4e2eb

kelseyyoung added 5 commits April 30, 2018 17:21

Merge with upstream master

fecba96

Renaming/refactoring to reduce bundle size

a02eec7

Merge with upstream master

579319e

Merge with upstream master

4b3ec81

Update max bundle size for Keytips

8edb33a

dzearing reviewed May 1, 2018

View reviewed changes

Comment thread scripts/tasks/jest-mock.js

up: 38,

right: 39,

down: 40,

insert: 45,

dzearing May 1, 2018

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

not sure what's up here.

dzearing reviewed May 1, 2018

View reviewed changes

jspurlin reviewed May 1, 2018

View reviewed changes

kelseyyoung added 2 commits May 1, 2018 11:15

Move keytip files from Utilities to fabric utilities

f649918

- Add HC styling for keytips when disabled - Restored missing KeyCodes

Merge remote-tracking branch 'upstream/master' into keyou/move-keytip…

ffeef79

…s-to-package

phkuo reviewed May 1, 2018

View reviewed changes

phkuo previously requested changes May 1, 2018

View reviewed changes

Removed keytip color from styling, using neutralTertiaryAlt instead

6f98ab2

kelseyyoung commented May 1, 2018

View reviewed changes

kelseyyoung added 3 commits May 1, 2018 13:02

Remove keytips comment in Semantic colors

10d5e98

Remove usage of isEqual

b878305

- Created arraysAreEqual utility function

Rename arraysAreEqual to arraysEqual

0c7899d

dzearing approved these changes May 1, 2018

View reviewed changes

dzearing merged commit 920a06d into microsoft:master May 1, 2018

cliffkoh added the 6.0-follow-up-required label May 3, 2018

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

Uh oh!

Conversation

kelseyyoung commented Apr 20, 2018 • edited 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!

jspurlin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelseyyoung May 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!

phkuo left a comment

Choose a reason for hiding this comment

Uh oh!

phkuo commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cliffkoh commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cliffkoh commented May 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kelseyyoung commented Apr 20, 2018 •

edited

Loading

kelseyyoung May 1, 2018 •

edited

Loading

cliffkoh commented May 3, 2018 •

edited

Loading