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

Conversation

@AquiGorka
Copy link
Member

commented Sep 3, 2019

The second commit (688ba39) is a first attempt to get the Popover's width and use that as the min-width for the container, but it only works after the first time the Popover appears.

Update: as discussed with @bpierre , followed the approach of having an invisible (out of viewport) content to get the width from.

@AquiGorka AquiGorka requested review from bpierre and sohkai Sep 3, 2019

@@ -118,6 +120,15 @@ const DropDown = React.memo(function DropDown({
return -1
}, [active, selected])

const [getContentWidth, setGetContentWidth] = useState(true)
const { refCallback: popoverRefCallback } = useButtonRef(el => {
setPlaceholderMinWidth(el.clientWidth)

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 3, 2019

Member

We need to adjust this width by 4GU to add extra room on the right for the caret

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 3, 2019

Author Member

2019-09-03 15 18 56

I don't think it is needed though

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 4, 2019

Member

Fixed in 638b9a7.

If you change this line to instead be width="50px" (or similarly small), this should be the correct result after selecting a long option:

Screen Shot 2019-09-04 at 5 21 22 PM

However, before 638b9a7, this is what would appear:

Screen Shot 2019-09-04 at 5 21 40 PM

src/components/DropDown/DropDown.js Show resolved Hide resolved
src/components/DropDown/DropDown.js Show resolved Hide resolved
src/components/DropDown/DropDown.js Show resolved Hide resolved

@AquiGorka AquiGorka force-pushed the design/dropdown branch from f0d80b9 to 0f2cf02 Sep 4, 2019

sohkai added 4 commits Sep 4, 2019
devbox/apps/DropDown.js Outdated Show resolved Hide resolved
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.

@bpierre
bpierre approved these changes Sep 4, 2019
Copy link
Member

left a comment

LGTM 🚀

@sohkai sohkai merged commit 593dbf7 into newstyle Sep 4, 2019

3 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
License Compliance All checks passed.
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the design/dropdown branch Sep 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.