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

Performance tuning #1274

Merged
merged 10 commits into from
Jul 22, 2021
8 changes: 5 additions & 3 deletions src/autocomplete/src/Autocomplete.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ const AutocompleteItems = ({
}
/* eslint-enable react/prop-types */

const containerStyle = { width: '100%' }

const Autocomplete = memo(
forwardRef(function Autocomplete(props, ref) {
const {
Expand Down Expand Up @@ -153,13 +155,13 @@ const Autocomplete = memo(
selectedItem,
...restDownshiftProps
}) => (
<div style={{ width: '100%' }}>
<div style={containerStyle}>
<Popover
bringFocusInside={false}
isShown={isShown}
minWidth={popoverMinWidth}
position={position || (targetWidth < popoverMinWidth ? Position.BOTTOM_LEFT : Position.BOTTOM)}
content={() => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unnecessary new function created every time. Unless we are using the close method we can simply pass the JSX directly

content={
<AutocompleteItems
getItemProps={getItemProps}
getMenuProps={getMenuProps}
Expand All @@ -176,7 +178,7 @@ const Autocomplete = memo(
title={props.title}
width={Math.max(targetWidth, popoverMinWidth)}
/>
)}
}
minHeight={0}
animationDuration={0}
>
Expand Down
9 changes: 6 additions & 3 deletions src/avatar/src/Avatar.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState, memo, forwardRef } from 'react'
import React, { useState, memo, forwardRef, useCallback } from 'react'
import cx from 'classnames'
import { css } from 'glamor'
import PropTypes from 'prop-types'
Expand All @@ -9,6 +9,8 @@ import { Text } from '../../typography'
import globalGetInitials from './utils/getInitials'
import globalHash from './utils/hash'

const imageStyles = { objectFit: 'cover' }

const pseudoSelectors = {}
const internalStyles = {
overflow: 'hidden',
Expand Down Expand Up @@ -62,6 +64,7 @@ const Avatar = memo(
)

const [imageHasFailedLoading, setImageHasFailedLoading] = useState(false)
const onError = useCallback(() => setImageHasFailedLoading(true), [])
Copy link
Contributor

Choose a reason for hiding this comment

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

this may not be as efficient as one would hope: https://kentcdodds.com/blog/usememo-and-usecallback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah for sure, might be too aggressive here. I was considering that there is internal state that will cause rerenders, and so we may as well cache the function, but it might be excessive in this case.

const imageUnavailable = !src || imageHasFailedLoading

const initialsFontSize = `${getAvatarInitialsFontSize(size, sizeLimitOneCharacter)}px`
Expand Down Expand Up @@ -95,11 +98,11 @@ const Avatar = memo(
)}
{!imageUnavailable && (
<Image
style={{ objectFit: 'cover' }} // Unsupported by ui-box directly
style={imageStyles} // Unsupported by ui-box directly
width={isObjectFitSupported ? '100%' : 'auto'} // Fallback to old behaviour on IE
height="100%"
src={src}
onError={() => setImageHasFailedLoading(true)}
onError={onError}
/>
)}
</Box>
Expand Down
8 changes: 2 additions & 6 deletions src/hooks/use-latest.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
import React from 'react'
import { useIsomorphicLayoutEffect } from './use-isomorphic-layout-effect'

/**
* A React Ref that stores the latest value it is given (updated in useEffect's callback).
* @return {{ readonly current: any }}
*/
export function useLatest(value) {
const ref = React.useRef(value)

useIsomorphicLayoutEffect(() => {
ref.current = value
})
Comment on lines -10 to -12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was unnecessary.


ref.current = value
return ref
}
101 changes: 60 additions & 41 deletions src/table/src/EditableCell.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, { memo, useEffect, useState } from 'react'
import React, { memo, useCallback, useEffect, useMemo, useRef, useState } from 'react'
import PropTypes from 'prop-types'
import { useLatest } from '../../hooks'
import safeInvoke from '../../lib/safe-invoke'
import { Portal } from '../../portal'
import { Stack } from '../../stack'
Expand All @@ -18,82 +19,100 @@ const EditableCell = memo(function EditableCell(props) {
isSelectable = true,
textProps = emptyProps,
autoFocus = false,
onChange,
...rest
} = props

let cursor = 'text'

const [mainRef, setMainRef] = useState()
const mainRef = useRef(null)
const [value, setValue] = useState(children)
const [isEditing, setIsEditing] = useState(autoFocus)
const onChangeRef = useLatest(onChange)

useEffect(() => {
setValue(children)
}, [children])

const handleDoubleClick = () => {
const handleDoubleClick = useCallback(() => {
if (disabled || !isSelectable) return

setIsEditing(true)
}
}, [disabled, isSelectable])

const handleKeyDown = useCallback(
e => {
if (disabled) return
const { key } = e

/**
* When the user presses a character on the keyboard, use that character
* as the value in the text field.
*/
if (key === 'Enter' || key === 'Shift') {
setIsEditing(true)
} else if (key.match(/^[a-z]{0,10}$/) && !e.metaKey && !e.ctrlKey && !e.altKey) {
setIsEditing(true)
setValue(prev => prev + key)
}
},
[disabled]
)

const handleKeyDown = e => {
if (disabled) return
const { key } = e

/**
* When the user presses a character on the keyboard, use that character
* as the value in the text field.
*/
if (key === 'Enter' || key === 'Shift') {
setIsEditing(true)
} else if (key.match(/^[a-z]{0,10}$/) && !e.metaKey && !e.ctrlKey && !e.altKey) {
setIsEditing(true)
setValue(value + key)
}
}
const handleFieldChangeComplete = useCallback(
value => {
setIsEditing(false)
setValue(value)

const handleFieldChangeComplete = value => {
const { onChange } = rest
safeInvoke(onChangeRef.current, value)

setIsEditing(false)
setValue(value)
if (mainRef.current && isSelectable) {
mainRef.current.focus()
}
},
// onChangeRef is a ref
// eslint-disable-next-line react-hooks/exhaustive-deps
[isSelectable]
)

safeInvoke(onChange, value)
const handleFieldCancel = useCallback(() => {
setIsEditing(false)
}, [])

if (mainRef && isSelectable) {
mainRef.focus()
const handleClick = useCallback(() => {
if (mainRef.current) {
mainRef.current.focus()
}
}
}, [])

const handleFieldCancel = () => {
setIsEditing(false)
}

const handleClick = () => {
if (mainRef) mainRef.focus()
}
const getTargetRef = useCallback(() => mainRef.current, [])

if (disabled) {
cursor = 'not-allowed'
} else if (isSelectable) {
cursor = 'default'
}

const lessOpacity = useMemo(() => disabled || (!value && placeholder), [disabled, value, placeholder])

const mergedTextProps = useMemo(
() => ({
size,
opacity: lessOpacity ? 0.5 : 1,
...textProps
}),
[lessOpacity, size, textProps]
mshwery marked this conversation as resolved.
Show resolved Hide resolved
)
return (
<React.Fragment>
<TextTableCell
ref={setMainRef}
ref={mainRef}
isSelectable={isSelectable}
onClick={handleClick}
onDoubleClick={handleDoubleClick}
onKeyDown={handleKeyDown}
cursor={cursor}
textProps={{
size,
opacity: disabled || (!value && placeholder) ? 0.5 : 1,
...textProps
}}
textProps={mergedTextProps}
{...rest}
>
{value || placeholder}
Expand All @@ -104,7 +123,7 @@ const EditableCell = memo(function EditableCell(props) {
{zIndex => (
<EditableCellField
zIndex={zIndex}
getTargetRef={() => mainRef}
getTargetRef={getTargetRef}
value={value}
onEscape={handleFieldCancel}
onChangeComplete={handleFieldChangeComplete}
Expand Down
Loading