Skip to content

feat(menus): update Menu/Select Item indentation and focus based on item hover #168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 16, 2018
Merged

feat(menus): update Menu/Select Item indentation and focus based on item hover #168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5504066
feat(menus): update Menu/Select Item indentation and focus based on i…
austingreendev Sep 28, 2018
40e2006
Format package.json
austingreendev Oct 10, 2018
f376545
Drop hover styling if currently focused
austingreendev Oct 10, 2018
08c9b29
Bump css-menus
austingreendev Oct 11, 2018
37eb24f
Merge branch 'master' into agreen/menu-indentation-styling
austingreendev Oct 16, 2018
981bd74
Update select documentation
austingreendev Oct 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/menus/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
},
"devDependencies": {
"@zendeskgarden/css-arrows": "3.1.1",
"@zendeskgarden/css-menus": "7.1.0",
"@zendeskgarden/css-menus": "8.0.2",
"@zendeskgarden/css-variables": "5.1.1",
"@zendeskgarden/react-theming": "^3.1.3",
"@zendeskgarden/svg-icons": "4.4.5"
Expand Down
4 changes: 1 addition & 3 deletions packages/menus/src/containers/MenuContainer.example.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,7 @@ initialState = {
placement="end"
onChange={selectedKey => setState({ selectedKey })}
trigger={({ getTriggerProps, triggerRef, isOpen }) => (
<button {...getTriggerProps({ ref: triggerRef, active: isOpen })}>
Heavily customized menu
</button>
<button {...getTriggerProps({ ref: triggerRef })}>Heavily customized menu</button>
)}
>
{({ getMenuProps, menuRef, placement, getItemProps, focusedKey }) => (
Expand Down
12 changes: 11 additions & 1 deletion packages/menus/src/containers/MenuContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ class MenuContainer extends ControlledComponent {
/**
* Props to be applied to each selectable menu item
**/
getItemProps = ({ key, role = 'menuitemcheckbox', textValue, ...other }) => {
getItemProps = ({ key, role = 'menuitemcheckbox', textValue, onMouseMove, ...other }) => {
const { focusedKey } = this.getControlledState();

if (typeof textValue === 'string' && textValue.length > 0) {
Expand Down Expand Up @@ -434,6 +434,16 @@ class MenuContainer extends ControlledComponent {
return {
key,
role,
/**
* onMouseMove is used as it is only triggered by actual mouse movement
*/
onMouseMove: composeEventHandlers(onMouseMove, () => {
Copy link
Contributor

@ryanseddon ryanseddon Oct 2, 2018

Choose a reason for hiding this comment

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

How come onMouseMove over onMouseEnter the latter would only fire once on entering the menu item.

Copy link
Contributor Author

@austingreendev austingreendev Oct 2, 2018

Choose a reason for hiding this comment

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

I originally had onMouseMove then, checked downshift for inspiration...

// onMouseMove is used over onMouseEnter here. onMouseMove
// is only triggered on actual mouse movement while onMouseEnter
// can fire on DOM changes, interrupting keyboard navigation

if (key !== focusedKey) {
this.setControlledState({
focusedKey: key
});
}
}),
...other
};
};
Expand Down
10 changes: 10 additions & 0 deletions packages/menus/src/containers/MenuContainer.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,16 @@ describe('MenuContainer', () => {
it('applies correct accessibility role', () => {
expect(findMenuItems(wrapper).first()).toHaveProp('role', 'menuitemcheckbox');
});

it('focuses correct item if mouseMoved', () => {
findMenuItems(wrapper)
.first()
.simulate('mousemove');

wrapper.update();

expect(findMenuItems(wrapper).first()).toHaveProp('data-focused', true);
});
});

describe('getNextItemProps()', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/menus/src/views/items/Item.example.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ const InlineMenuView = styled(MenuView)`
</Col>
<Col md>
<InlineMenuView>
<HeaderItem>
<HeaderItem containsIcon>
<HeaderIcon>
<GroupIcon />
</HeaderIcon>
Expand Down
9 changes: 9 additions & 0 deletions packages/menus/src/views/items/Item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,16 @@ const Item = styled.li.attrs({
[MenuStyles['is-checked']]: props.checked
})
})`
/* stylelint-disable */
${props =>
!props.focused &&
`&&&:hover {
Copy link
Member

@jzempel jzempel Oct 10, 2018

Choose a reason for hiding this comment

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

gnarly, but effective!

background-color: inherit;
}
`};

${props => retrieveTheme(COMPONENT_ID, props)};
/* stylelint-enable */
`;

Item.propTypes = {
Expand Down
30 changes: 25 additions & 5 deletions packages/menus/src/views/items/__snapshots__/Item.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,49 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Item renders active styling 1`] = `
.c0.c0.c0:hover {
background-color: inherit;
}

<li
className="c-menu__item is-active "
className="c-menu__item is-active c0"
data-garden-id="menus.item"
data-garden-version="version"
/>
`;

exports[`Item renders checked styling 1`] = `
.c0.c0.c0:hover {
background-color: inherit;
}

<li
checked={true}
className="c-menu__item is-checked "
className="c-menu__item is-checked c0"
data-garden-id="menus.item"
data-garden-version="version"
/>
`;

exports[`Item renders default styling 1`] = `
.c0.c0.c0:hover {
background-color: inherit;
}

<li
className="c-menu__item "
className="c-menu__item c0"
data-garden-id="menus.item"
data-garden-version="version"
/>
`;

exports[`Item renders disabled styling 1`] = `
.c0.c0.c0:hover {
background-color: inherit;
}

<li
className="c-menu__item is-disabled "
className="c-menu__item is-disabled c0"
data-garden-id="menus.item"
data-garden-version="version"
disabled={true}
Expand All @@ -43,8 +59,12 @@ exports[`Item renders focused styling 1`] = `
`;

exports[`Item renders hovered styling 1`] = `
.c0.c0.c0:hover {
background-color: inherit;
}

<li
className="c-menu__item is-hovered "
className="c-menu__item is-hovered c0"
data-garden-id="menus.item"
data-garden-version="version"
/>
Expand Down
10 changes: 8 additions & 2 deletions packages/menus/src/views/items/header/HeaderItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import PropTypes from 'prop-types';
import className from 'classnames';
import styled from 'styled-components';
import { retrieveTheme } from '@zendeskgarden/react-theming';
import MenuStyles from '@zendeskgarden/css-menus';
Expand All @@ -20,7 +21,10 @@ const COMPONENT_ID = 'menus.header_item';
const HeaderItem = styled(Item).attrs({
'data-garden-id': COMPONENT_ID,
'data-garden-version': PACKAGE_VERSION,
className: MenuStyles['c-menu__item--header']
className: props =>
className(MenuStyles['c-menu__item--header'], {
[MenuStyles['c-menu__item--header--icon']]: props.containsIcon
})
})`
${props => retrieveTheme(COMPONENT_ID, props)};
`;
Expand All @@ -30,7 +34,9 @@ HeaderItem.propTypes = {
focused: PropTypes.bool,
hovered: PropTypes.bool,
disabled: PropTypes.bool,
checked: PropTypes.bool
checked: PropTypes.bool,
/** Applies icon styling */
containsIcon: PropTypes.bool
};

HeaderItem.hasType = () => HeaderItem;
Expand Down
10 changes: 10 additions & 0 deletions packages/menus/src/views/items/header/HeaderItem.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,14 @@ describe('HeaderItem', () => {

expect(wrapper).toMatchSnapshot();
});

it('renders icon styling if provided', () => {
const wrapper = shallow(
<HeaderItem containsIcon>
<svg />
</HeaderItem>
);

expect(wrapper).toMatchSnapshot();
});
});
11 changes: 11 additions & 0 deletions packages/menus/src/views/items/header/__snapshots__/HeaderItem.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,14 @@ exports[`HeaderItem renders default styling 1`] = `
data-garden-version="version"
/>
`;

exports[`HeaderItem renders icon styling if provided 1`] = `
<Item
className="c-menu__item--header c-menu__item--header--icon "
containsIcon={true}
data-garden-id="menus.header_item"
data-garden-version="version"
>
<svg />
</Item>
`;
2 changes: 1 addition & 1 deletion packages/select/src/views/Dropdown.example.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ const InlineSelectView = styled(Dropdown)`
</Col>
<Col md>
<InlineSelectView>
<HeaderItem>
<HeaderItem containsIcon>
<HeaderIcon>
<GroupIcon />
</HeaderIcon>
Expand Down