Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Prototypes): add MenuButton #947

Merged
merged 12 commits into from
Feb 27, 2019
Merged

feat(Prototypes): add MenuButton #947

merged 12 commits into from
Feb 27, 2019

Conversation

layershifter
Copy link
Member

Fixes #877.

@codecov
Copy link

codecov bot commented Feb 22, 2019

Codecov Report

Merging #947 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #947      +/-   ##
==========================================
+ Coverage   80.51%   80.51%   +<.01%     
==========================================
  Files         659      659              
  Lines        8448     8449       +1     
  Branches     1492     1429      -63     
==========================================
+ Hits         6802     6803       +1     
  Misses       1631     1631              
  Partials       15       15
Impacted Files Coverage Δ
...b/accessibility/Behaviors/Menu/menuItemBehavior.ts 100% <100%> (ø) ⬆️
packages/react/src/index.ts 100% <100%> (ø) ⬆️

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 ccc755b...25a615f. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 22, 2019

Codecov Report

Merging #947 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #947   +/-   ##
=====================================
  Coverage      81%    81%           
=====================================
  Files         665    665           
  Lines        8528   8528           
  Branches     1443   1443           
=====================================
  Hits         6908   6908           
  Misses       1606   1606           
  Partials       14     14

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 82266e9...173bcf0. Read the comment docs.

@kolaps33
Copy link
Collaborator

nice work :) 👍
tested from accessibility point of view, it works as expected... Keyboard behavior as well reader experience.

docs/src/prototypes/MenuButton/MenuButton.tsx Outdated Show resolved Hide resolved
docs/src/prototypes/MenuButton/MenuButton.tsx Outdated Show resolved Hide resolved
docs/src/prototypes/MenuButton/MenuButton.tsx Outdated Show resolved Hide resolved
docs/src/prototypes/MenuButton/MenuButton.tsx Outdated Show resolved Hide resolved
}
}

handleMenuItemOverrides = accessibilityBehavior =>
Copy link
Contributor

Choose a reason for hiding this comment

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

lets accept menuItemAccessibilityBehavior as an argument of this method: this will decouple it from the parent accessibility object of menuButton, as well as will make it easier to infer the intent at the place of usage:

Menu.create(menu, {
   defaultProps: {
      ...accessibilityBehavior.attributes.menu,
     overrideProps: {
       items: this.handleMenuItemOverrides(accessibilityBehavior.menuItem),
       ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Reworked this part, now we're passing: (menuItemAccessibilityAttributes: AccessibilityAttributes) => {}

docs/src/prototypes/MenuButton/MenuButton.tsx Outdated Show resolved Hide resolved
docs/src/prototypes/MenuButton/index.tsx Show resolved Hide resolved
docs/src/prototypes/MenuButton/MenuButton.tsx Outdated Show resolved Hide resolved
getPreviousElement,
focusAsync,
} from './lib/accessibility/FocusZone/focusUtilities'
export const focusZoneUtilities = {
Copy link
Contributor

@kuzhelov kuzhelov Feb 27, 2019

Choose a reason for hiding this comment

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

actually, this is a breaking change :( My full support to introduce it, but either

  • by changing changelog entry accordingly
  • by means of dedicated PR

import { focusZoneUtilities, Menu } from '@stardust-ui/react'

export const focusMenuItem = (menuRef: HTMLUListElement, order: 'first' | 'last') => {
const selector = `.${Menu.Item.slotClassNames.wrapper}:${order}-child .${Menu.Item.className}`
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this is something that might strike a user, as .${Menu.Item.className}:${order}-child is the most intuitive thing to try

…stardust-ui/react into feat/menu-button

# Conflicts:
#	CHANGELOG.md
#	packages/react/src/index.ts
@layershifter layershifter merged commit 6036da4 into master Feb 27, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/menu-button branch February 27, 2019 16:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants