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

fix: use and mock lodash debounce in Carousel and Dropdown #2203

Merged
merged 6 commits into from
Jan 7, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Fix `toggle` changing width during animation in Teams theme @mnajdova ([#2189](https://github.com/microsoft/fluent-ui-react/pull/2189))
- Fix `Popup` positioning in multiple cases @layershifter ([#2187](https://github.com/microsoft/fluent-ui-react/pull/2187))
- Fix click outside in `Popup` when `trigger` is not defined @layershifter ([#2202](https://github.com/microsoft/fluent-ui-react/pull/2202))
- Use `debounce` from `lodash` in `Dropdown` and `Carouel` @silviuavram ([#2203](https://github.com/microsoft/fluent-ui-react/pull/2203))

<!--------------------------------[ v0.42.0 ]------------------------------- -->
## [v0.42.0](https://github.com/microsoft/fluent-ui-react/tree/v0.42.0) (2019-12-12)
Expand Down
8 changes: 5 additions & 3 deletions packages/react/src/components/Carousel/Carousel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
ChildrenComponentProps,
getOrGenerateIdFromShorthand,
AutoControlledComponent,
debounce,
} from '../../utils'
import {
WithAsProp,
Expand Down Expand Up @@ -171,6 +170,10 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous
}
}

componentWillUnmount() {
this.focusItemAtIndex.cancel()
}

actionHandlers = {
showNextSlideByKeyboardNavigation: e => {
e.preventDefault()
Expand Down Expand Up @@ -213,8 +216,7 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous
itemRefs = [] as React.RefObject<HTMLElement>[]
paddleNextRef = React.createRef<HTMLElement>()
paddlePreviousRef = React.createRef<HTMLElement>()

focusItemAtIndex = debounce((index: number) => {
focusItemAtIndex = _.debounce((index: number) => {
Copy link
Member

@dzearing dzearing Jan 7, 2020

Choose a reason for hiding this comment

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

Minor nullref edge case if you don't cancel this.

this.itemRefs[index].current.focus()
}, 400)

Expand Down
30 changes: 15 additions & 15 deletions packages/react/src/components/Dropdown/Dropdown.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,11 @@ class Dropdown extends AutoControlledComponent<WithAsProp<DropdownProps>, Dropdo
static SearchInput = DropdownSearchInput
static SelectedItem = DropdownSelectedItem

componentWillUnmount() {
this.clearStartingString.cancel()
this.clearA11ySelectionMessage.cancel()
}

getInitialAutoControlledState({ multiple, search }: DropdownProps): DropdownState {
return {
a11ySelectionStatus: '',
Expand All @@ -361,15 +366,8 @@ class Dropdown extends AutoControlledComponent<WithAsProp<DropdownProps>, Dropdo
}
}

a11yStatusTimeout: any
charKeysPressedTimeout: any
defaultTriggerButtonId = _.uniqueId('dropdown-trigger-button-')

componentWillUnmount() {
clearTimeout(this.a11yStatusTimeout)
clearTimeout(this.charKeysPressedTimeout)
}
Copy link
Member

Choose a reason for hiding this comment

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

Please add an cancel() call for _.debounce:

https://lodash.com/docs/4.17.15#debounce

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you mean this.clearStartingString.cancel() ?

Copy link
Member

@dzearing dzearing Jan 7, 2020

Choose a reason for hiding this comment

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

I believe this.focusItemAtIndex.cancel() as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Solved. Thanks!


/**
* Used to compute the filtered items (by value and search query) and, if needed,
* their string equivalents, in order to be used throughout the component.
Expand Down Expand Up @@ -1348,20 +1346,22 @@ class Dropdown extends AutoControlledComponent<WithAsProp<DropdownProps>, Dropdo
* so it is not read anymore via virtual cursor.
*/
setA11ySelectionMessage = (a11ySelectionStatus: string): void => {
clearTimeout(this.a11yStatusTimeout)
this.setState({ a11ySelectionStatus })
this.a11yStatusTimeout = setTimeout(() => {
this.setState({ a11ySelectionStatus: '' })
}, Dropdown.a11yStatusCleanupTime)
this.clearA11ySelectionMessage()
}

setStartingString = (startingString: string): void => {
clearTimeout(this.charKeysPressedTimeout)
this.setState({ startingString })
this.charKeysPressedTimeout = setTimeout(() => {
this.setState({ startingString: '' })
}, Dropdown.charKeyPressedCleanupTime)
this.clearStartingString()
}

clearA11ySelectionMessage = _.debounce(() => {
this.setState({ a11ySelectionStatus: '' })
}, Dropdown.a11yStatusCleanupTime)

clearStartingString = _.debounce(() => {
this.setState({ startingString: '' })
}, Dropdown.charKeyPressedCleanupTime)
}

Dropdown.slotClassNames = {
Expand Down
1 change: 0 additions & 1 deletion packages/react/src/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,3 @@ export module commonPropTypes {

export { default as withDebugId } from './withDebugId'
export { default as Telemetry } from './Telemetry'
export { default as debounce } from './debounce'
Original file line number Diff line number Diff line change
@@ -1,23 +1,19 @@
const debounce = (fn: Function, time: number): Function => {
// @ts-ignore
export * from 'lodash'
export const debounce = (fn: Function, time: number): Function => {
let timeoutId

function cancel() {
if (timeoutId) {
clearTimeout(timeoutId)
}
}

function wrapper(...args) {
cancel()
timeoutId = setTimeout(() => {
timeoutId = null
fn(...args)
}, time)
}

wrapper.cancel = cancel

return wrapper
}

export default debounce
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { ShorthandValue } from 'src/types'

jest.dontMock('keyboard-key')
jest.useFakeTimers()
// jest.mock('lodash')

const getTriggerButtonWrapper = (wrapper: ReactWrapper): CommonWrapper =>
findIntrinsicElement(wrapper, `.${Dropdown.slotClassNames.triggerButton}`)
Expand Down