Skip to content
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.

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

Feature/enyo 4811 roysutton #1301

Merged
merged 19 commits into from
Nov 16, 2017
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
4 changes: 4 additions & 0 deletions packages/moonstone/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ The following is a curated list of changes in the Enact moonstone module, newest
- `moonstone/VirtualList` to scroll correctly using page down key with disabled items
- `moonstone/Scrollable` to not cause a script error when scrollbar is not rendered
- `moonstone/Picker` incrementer and decrementer to not change size when focused
- `moonstone/Marquee` to correctly start when hovering on disabled spottable components
- `moonstone/Marquee.MarqueeController` to not abort marquee when moving among components
- `moonstone/Picker` marquee issues with disabled buttons or Picker
- `moonstone/LabeledItem` to start marquee when hovering over in disabled state

## [1.12.1] - 2017-11-07

Expand Down
16 changes: 12 additions & 4 deletions packages/moonstone/LabeledItem/LabeledItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ const LabeledItemBase = kind({
*/
children: PropTypes.node.isRequired,

/**
* When `true`, applies a disabled style and the control becomes non-interactive.
*
* @type {Boolean}
* @public
*/
disabled: PropTypes.bool,

/**
* The label to be displayed along with the text.
*
Expand All @@ -63,13 +71,13 @@ const LabeledItemBase = kind({
className: 'labeleditem'
},

render: ({children, label, titleIcon, ...rest}) => (
<Controller {...rest}>
render: ({children, disabled, label, titleIcon, ...rest}) => (
<Controller disabled={disabled} {...rest}>
<div className={css.text}>
<MarqueeText className={css.title}>{children}</MarqueeText>
<MarqueeText disabled={disabled} className={css.title}>{children}</MarqueeText>
{(titleIcon != null) ? <Icon small className={css.icon}>{titleIcon}</Icon> : null}
</div>
{(label != null) ? <MarqueeText className={css.label}>{label}</MarqueeText> : null}
{(label != null) ? <MarqueeText disabled={disabled} className={css.label}>{label}</MarqueeText> : null}
</Controller>
)
});
Expand Down
4 changes: 2 additions & 2 deletions packages/moonstone/LabeledItem/tests/LabeledItem-specs.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';
import {mount} from 'enzyme';
import {mount, shallow} from 'enzyme';

import LabeledItem from '../LabeledItem';
import css from '../LabeledItem.less';
Expand Down Expand Up @@ -44,7 +44,7 @@ describe('LabeledItem Specs', () => {
});

it('should have \'disabled\' HTML attribute when \'disabled=true\'', function () {
const item = mount(
const item = shallow(
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming we couldn't do a shallow test here, there should be a cleaner way to write this test so that we aren't checking the specific number of elements (1) with a disabled prop set to true.

Copy link
Contributor

Choose a reason for hiding this comment

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

<LabeledItem disabled>I am a disabled labeledItem</LabeledItem>
);

Expand Down
64 changes: 58 additions & 6 deletions packages/moonstone/Marquee/MarqueeController.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {forward} from '@enact/core/handle';
import hoc from '@enact/core/hoc';
import {Job} from '@enact/core/util';
import React from 'react';
import PropTypes from 'prop-types';

Expand Down Expand Up @@ -33,6 +34,22 @@ const contextTypes = {
*/
complete: PropTypes.func,

/**
* Called by Marquee instances when hovered
*
* @type {Function}
* @memberof moonstone/Marquee.Marquee.contextTypes
*/
enter: PropTypes.func,

/**
* Called by Marquee instances when unhovered
*
* @type {Function}
* @memberof moonstone/Marquee.Marquee.contextTypes
*/
leave: PropTypes.func,

/**
* Called to register a Marquee instance to be synchronized
*
Expand Down Expand Up @@ -111,12 +128,20 @@ const MarqueeController = hoc(defaultConfig, (config, Wrapped) => {
return {
cancel: this.handleCancel,
complete: this.handleComplete,
enter: this.handleEnter,
leave: this.handleLeave,
register: this.handleRegister,
start: this.handleStart,
unregister: this.handleUnregister
};
}

componentWillUnmount () {
this.cancelJob.stop();
}

cancelJob = new Job(() => this.doCancel(), 30)
Copy link
Contributor

Choose a reason for hiding this comment

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

30 seems like a magic number - what was the reasoning behind the specific value?

Copy link
Contributor Author

@webOS101 webOS101 Nov 15, 2017

Choose a reason for hiding this comment

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

It is a bit magical. It was supposed to be long enough to make it async but not so long that the user would notice a lag between unhovering/unspotting and the marquee stopping. It should be long enough that as the move they pointer between the different parts of a controlled set that it wouldn't reset. I was thinking 100-300ms was probably a reasonable number but 30 seems to work well in testing. It may be possible to go without it given the ordering of the events but I didn't want to rely on possibly unpredictable event ordering.

Copy link
Contributor

@aarontam aarontam Nov 16, 2017

Choose a reason for hiding this comment

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

🎩 🐰

Admittedly feels a bit dirty...


/*
* Registers `component` with a set of handlers for `start` and `stop`.
*
Expand Down Expand Up @@ -169,6 +194,7 @@ const MarqueeController = hoc(defaultConfig, (config, Wrapped) => {
* @returns {undefined}
*/
handleStart = (component) => {
this.cancelJob.stop();
if (!this.anyRunning()) {
this.markAll(STATE.ready);
this.dispatch('start', component);
Expand All @@ -182,9 +208,16 @@ const MarqueeController = hoc(defaultConfig, (config, Wrapped) => {
*
* @returns {undefined}
*/
handleCancel = (component) => {
handleCancel = () => {
this.cancelJob.start();
}

doCancel = () => {
if (this.isHovered || this.isFocused) {
return;
}
this.markAll(STATE.inactive);
this.dispatch('stop', component);
this.dispatch('stop');
}

/*
Expand All @@ -197,17 +230,34 @@ const MarqueeController = hoc(defaultConfig, (config, Wrapped) => {
handleComplete = (component) => {
const complete = this.markReady(component);
if (complete) {
this.cancelJob.stop();
this.markAll(STATE.ready);
this.dispatch('start');
}
}

handleEnter = () => {
this.isHovered = true;
if (!this.anyRunning()) {
this.dispatch('start');
}
this.cancelJob.stop();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to stop the job if we're hovered now? Will the job's fn exit early?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 We -could- rely on the job to run out and be trapped by the hover event. It seemed wiser to always clean up the job than to rely on logic elsewhere. I'm game for removing these extra ones.

}

handleLeave = () => {
this.isHovered = false;
this.cancelJob.start();
}

/*
* Handler for the focus event
*/
handleFocus = (ev) => {
this.isFocused = true;
this.dispatch('start');
if (!this.anyRunning()) {
this.dispatch('start');
}
this.cancelJob.stop();
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar question here as for handleEnter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar answer here as for handleEnter.

forwardFocus(ev, this.props);
}

Expand All @@ -216,8 +266,7 @@ const MarqueeController = hoc(defaultConfig, (config, Wrapped) => {
*/
handleBlur = (ev) => {
this.isFocused = false;
this.dispatch('stop');
this.markAll(STATE.inactive);
this.cancelJob.start();
forwardBlur(ev, this.props);
}

Expand Down Expand Up @@ -312,7 +361,10 @@ const MarqueeController = hoc(defaultConfig, (config, Wrapped) => {
props = {
...this.props,
onBlur: this.handleBlur,
onFocus: this.handleFocus
onFocus: this.handleFocus,
// When picker button becomes disabled, it doesn't fire blur, but does fire
// `onSpotlightDisappear`. We should investigate why `onBlur` does not fire
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting find! Could be related. Talked some with @germboy about it but we didn't reach any conclusions.

onSpotlightDisappear: this.handleBlur
};
}

Expand Down
50 changes: 35 additions & 15 deletions packages/moonstone/Marquee/MarqueeDecorator.js
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,8 @@ const MarqueeDecorator = hoc(defaultConfig, (config, Wrapped) => {
this.textDirectionValidated = false;
} else if (next.marqueeOn !== marqueeOn || next.marqueeDisabled !== marqueeDisabled || next.marqueeSpeed !== marqueeSpeed) {
this.cancelAnimation();
} else if (next.disabled && this.isHovered && marqueeOn === 'focus' && this.sync) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain how the marqueeOn === 'focus' check helps in this scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this scenario, we're checking for a transition to disabled when we were in 'focus' start. In this case, we could get a 'blur' event, which would cancel the marquee. Since we're hovered, we want to make sure the marquee continues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not to mention that we don't fire the 'enter' event when focused and enabled.

this.context.enter(this);
}
}

Expand Down Expand Up @@ -372,7 +374,8 @@ const MarqueeDecorator = hoc(defaultConfig, (config, Wrapped) => {
this.forceRestartMarquee ||
!this.sync && (
(this.isFocused && this.props.marqueeOn === 'focus') ||
(this.isHovered && this.props.marqueeOn === 'hover')
(this.isHovered && this.props.marqueeOn === 'hover') ||
(this.isHovered && this.props.marqueeOn === 'focus' && this.props.disabled)
)
);
}
Expand Down Expand Up @@ -568,6 +571,7 @@ const MarqueeDecorator = hoc(defaultConfig, (config, Wrapped) => {
cancelAnimation = () => {
if (this.sync) {
this.context.cancel(this);
return;
}

this.stop();
Expand All @@ -592,35 +596,51 @@ const MarqueeDecorator = hoc(defaultConfig, (config, Wrapped) => {

handleFocus = (ev) => {
this.isFocused = true;
this.setState((prevState) => {
if (!prevState.animating) {
this.startAnimation();
}
return null;
});
if (!this.sync) {
this.setState((prevState) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth throwing this into a fn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:indeed:

if (!prevState.animating) {
this.startAnimation();
}
return null;
});
}
forwardFocus(ev, this.props);
}

handleBlur = (ev) => {
this.isFocused = false;
this.cancelAnimation();
if (!this.sync) {
this.cancelAnimation();
}
forwardBlur(ev, this.props);
}

handleEnter = (ev) => {
this.isHovered = true;
this.setState((prevState) => {
if (!prevState.animating) {
this.startAnimation();
if (this.props.disabled || this.props.marqueeOn === 'hover') {
if (this.sync) {
this.context.enter(this);
} else {
this.setState((prevState) => {
if (!prevState.animating) {
this.startAnimation();
}
return null;
});
}
return null;
});
}
forwardEnter(ev, this.props);
}

handleLeave = (ev) => {
this.isHovered = false;
this.cancelAnimation();
if (this.props.disabled || this.props.marqueeOn === 'hover') {
if (this.sync) {
this.context.leave(this);
} else {
this.cancelAnimation();
}
}
forwardLeave(ev, this.props);
}

Expand Down Expand Up @@ -669,7 +689,7 @@ const MarqueeDecorator = hoc(defaultConfig, (config, Wrapped) => {
}

// TODO: cancel others on hover
if (marqueeOnHover || (disabled && marqueeOnFocus)) {
if (marqueeOnHover || marqueeOnFocus) {
rest[enter] = this.handleEnter;
rest[leave] = this.handleLeave;
}
Expand Down
1 change: 1 addition & 0 deletions packages/moonstone/internal/Picker/Picker.less
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
border-width: 0;
border-style: solid;
border-radius: inherit;
pointer-events: none;
}

.icon {
Expand Down
29 changes: 28 additions & 1 deletion packages/moonstone/internal/Picker/PickerButton.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import Holdable from '../Holdable';
import {forward, handle} from '@enact/core/handle';
import kind from '@enact/core/kind';
import React from 'react';
import PropTypes from 'prop-types';
import Pure from '@enact/ui/internal/Pure';

import {contextTypes} from '../../Marquee/MarqueeController';
import Holdable from '../Holdable';
import Icon from '../../Icon';
import IconButton from '../../IconButton';
import {withSkinnableProps} from '../../Skinnable';
Expand All @@ -26,10 +28,35 @@ const PickerButtonBase = kind({
spotlightDisabled: PropTypes.bool
},

defaultProps: {
joined: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not any longer since I removed forProp(). Just forgot to clean it up.

},

contextTypes: contextTypes,

styles: {
css
},

handlers: {
onMouseEnter: handle(
forward('onMouseEnter'),
(ev, props, context) => {
if (context.enter) {
context.enter(null);
}
}
),
onMouseLeave: handle(
forward('onMouseLeave'),
(ev, props, context) => {
if (context.leave) {
context.leave(null);
}
}
)
},

computed: {
className: ({hidden, styler}) => styler.append({
hidden
Expand Down
1 change: 1 addition & 0 deletions packages/spotlight/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ The following is a curated list of changes in the Enact spotlight module, newest
### Fixed

- `spotlight` to handle non-5-way keys correctly to focus on next 5-way keys
- `spotlight/Spottable` to forward `onMouseEnter` and `onMouseLeave`

## [1.12.1] - 2017-11-07

Expand Down
6 changes: 4 additions & 2 deletions packages/spotlight/Spottable/Spottable.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,11 +321,13 @@ const Spottable = hoc(defaultConfig, (config, Wrapped) => {
}
}

handleEnter = () => {
handleEnter = (ev) => {
forward('onMouseEnter', ev, this.props);
this.isHovered = true;
}

handleLeave = () => {
handleLeave = (ev) => {
forward('onMouseLeave', ev, this.props);
this.isHovered = false;
}

Expand Down