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

Design polish dropdown #559

Merged
merged 12 commits into from Sep 4, 2019
@@ -14,7 +14,14 @@ const items = [
justifyContent: 'space-between',
}}
>
<span>Test</span>
<span
css={`
/* Adjust for no descenders (due to textstyle: uppercase) */
padding-top: 4px;
This conversation was marked as resolved by sohkai

This comment has been minimized.

Copy link
@bpierre

bpierre Sep 4, 2019

Member

I don’t think we should use an uppercase text style in the DropDown?

`}
>
Test
</span>
<IdentityBadge entity="0xc41e4c10b37d3397a99d4a90e7d85508a69a5c4c" />
</span>,
]
@@ -8,6 +8,8 @@ import { useViewport } from '../../providers/Viewport/Viewport'
import ButtonBase from '../ButtonBase/ButtonBase'
import Popover from '../Popover/Popover'

const MIN_WIDTH = 128

function useDropDown({
buttonRef,
items,
@@ -107,32 +109,51 @@ const DropDown = React.memo(function DropDown({
}

const theme = useTheme()
const { width: vw } = useViewport()

const selectedIndex = useMemo(() => {
if (selected !== undefined) {
return selected
}
if (active !== undefined) {
return active
}
return -1
}, [active, selected])

const [widthNoPx = MIN_WIDTH] = (width || '').split('px')
const [placeholderMinWidth, setPlaceholderMinWidth] = useState(
Math.min(widthNoPx, MIN_WIDTH)
)
const [buttonWidth, setButtonWidth] = useState(0)
const [getContentWidth, setGetContentWidth] = useState(true)
This conversation was marked as resolved by AquiGorka

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 3, 2019

Member

Should we also reset getContentWidth to true if the items changes?


// Adjust the button width if the item widths are larger than declared width
const popoverRefCallback = useCallback(el => {
if (!el) {
return
}
// Add 4 GU to accomodate for caret spacing
setPlaceholderMinWidth(el.clientWidth + 4 * GU)
setGetContentWidth(false)
This conversation was marked as resolved by sohkai

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 3, 2019

Member

Perhaps we can move this logic to be closer to the other useButtonRef, since it's also doing a resize operation?

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 3, 2019

Author Contributor

Not sure if it can be closer, each callback has different references, or maybe I misunderstand, what do you mean by closer?

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 4, 2019

Member

Just moving it close to the other logic (e.g. useEffect()) that are about resizing.

}, [])
// Re-adjust if the width or items ever change
useEffect(() => {
setGetContentWidth(true)
}, [vw, items])

// Update the button width every time the reference updates
const { refCallback, buttonRef } = useButtonRef(el => {
// Update the button width every time the reference updates
setButtonWidth(el.clientWidth)
})

// And every time the viewport resizes
const { width: vw } = useViewport()
useEffect(() => {
if (buttonRef.current) {
setButtonWidth(buttonRef.current.clientWidth)
if (!buttonRef.current) {
return
}
setButtonWidth(buttonRef.current.clientWidth)
}, [buttonRef, vw])

const selectedIndex = useMemo(() => {
if (selected !== undefined) {
return selected
}
if (active !== undefined) {
return active
}
return -1
}, [active, selected])

const {
handleItemSelect,
close,
@@ -163,13 +184,15 @@ const DropDown = React.memo(function DropDown({
justify-content: space-between;
align-items: center;
height: ${5 * GU}px;
padding: 0 ${2 * GU}px;
padding-left: ${2 * GU}px;
padding-right: ${1.5 * GU}px;
width: ${width || (wide ? '100%' : 'auto')};
min-width: ${placeholderMinWidth}px;
background: ${disabled ? theme.disabled : theme.surface};
color: ${disabled ? theme.disabledContent : theme.surfaceContent};
border: ${disabled ? 0 : 1}px solid
${closedWithChanges ? theme.selected : theme.border};
${textStyle('body2')};
${textStyle('label2')};

This comment has been minimized.

Copy link
@bpierre

bpierre Sep 4, 2019

Member

I don’t think this is the right style (it shouldn’t be uppercase).

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 4, 2019

Member

Indeed, fixed in 8689c72.

${disabled
? 'font-weight: 600;'
: `
@@ -184,54 +207,37 @@ const DropDown = React.memo(function DropDown({
<IconDown
size="tiny"
css={`
margin-left: ${1 * GU}px;
margin-left: ${1.5 * GU}px;
color: ${closedWithChanges && !disabled ? theme.accent : 'inherit'};
`}
/>
</ButtonBase>
<Popover onClose={close} opener={buttonRef.current} visible={opened}>
{getContentWidth && (
<div
css={`
min-width: ${buttonWidth}px;
color: ${theme.surfaceContentSecondary};
position: absolute;
top: -100vh;
left: -100vw;
This conversation was marked as resolved by AquiGorka

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 3, 2019

Member

Could we add an opacity: 0 or visibliity: hidden?

opacity: 0;
visibility: hidden;
`}
>
{header && (
<div
css={`
padding: ${1.5 * GU}px ${2 * GU}px ${1.25 * GU}px;
${textStyle('label2')};
${unselectable};
`}
>
{header}
</div>
)}
<ul
css={`
margin: 0;
list-style: none;
width: 100%;
`}
>
<Inside name="DropDown:menu">
{items.map((item, index) => {
return (
<Item
key={index}
index={index}
onSelect={handleItemSelect}
theme={theme}
item={item}
header={header}
length={items.length}
selected={selectedIndex}
/>
)
})}
</Inside>
</ul>
<PopoverContent
refCallback={popoverRefCallback}
buttonWidth={buttonWidth}
header={header}
items={items}
/>
</div>
)}
<Popover onClose={close} opener={buttonRef.current} visible={opened}>
<PopoverContent
buttonWidth={buttonWidth}
header={header}
items={items}
handleItemSelect={handleItemSelect}
selectedIndex={selectedIndex}
/>
</Popover>
</Inside>
)
@@ -259,6 +265,77 @@ DropDown.defaultProps = {
wide: false,
}

const PopoverContent = React.memo(function PopoverContent({
refCallback,
buttonWidth,
header,
items,
handleItemSelect,
selectedIndex,
}) {
const theme = useTheme()
return (
<div
ref={refCallback}
css={`
min-width: ${buttonWidth}px;
color: ${theme.surfaceContentSecondary};
`}
>
{header && (
<div
css={`
padding: ${1.5 * GU}px ${2 * GU}px ${1.25 * GU}px;
${textStyle('label2')};
${unselectable};
`}
>
{header}
</div>
)}
<ul
css={`
margin: 0;
list-style: none;
width: 100%;
`}
>
<Inside name="DropDown:menu">
{items.map((item, index) => {
return (
<Item
key={index}
index={index}
onSelect={handleItemSelect}
theme={theme}
item={item}
header={header}
length={items.length}
selected={selectedIndex}
/>
)
})}
</Inside>
</ul>
</div>
)
})

PopoverContent.propTypes = {
refCallback: PropTypes.func.isRequired,
buttonWidth: PropTypes.number.isRequired,
header: PropTypes.node.isRequired,
items: PropTypes.array.isRequired,
handleItemSelect: PropTypes.func.isRequired,
selectedIndex: PropTypes.number.isRequired,
}

PopoverContent.defaultProps = {
refCallback: () => null,
handleItemSelect: () => null,
selectedIndex: -1,
}

const Item = React.memo(function Item({
header,
index,
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.