Update overlay components to new theme system #209

lederer · 2019-07-26T18:16:11Z

Target Branch

Note this is a request to merge to feature/theme, not develop. All theme-related PRs should target feature/theme until the theme upgrade is complete.

Overview

Updates the following components to the new theme system:

Tooltip
Dialog

Updates the Dialog docs page with a fully working example and clearer instructions for how to close the dialog.

Checklist

Relevant documentation pages have been created or updated
Description of PR is in an appropriate section of the changelog and grouped with similar changes if possible
Relevant status has been updated on the status page

Upgrade instructions

If there are any of the following in this PR, provide proper instructions on how to upgrade:

Changes to the prop names or types of existing components
Changes in intended behavior of existing component props
Changes in the theme file's structure
A required version bump to a non-dev dependency of the project

Notes

Did a some drive-by tweaks to Callout, Breadcrumbs, Button.

This PR does not change the underlying tooltip and dialog libraries used by Tooltip and Dialog.

Testing Instructions

Fiddle with the corresponding docs pages

Closes #171
Closes #172
Closes #131
Closes #205

lederer · 2019-07-29T14:51:09Z

Just added an unrelated fix for Checkbox and Radio vertical alignment.

lederer · 2019-07-29T15:58:56Z

Just added an unrelated fix for Grouper alignment.

designmatty · 2019-07-29T14:39:47Z

packages/core/components/dialog/index.js

@@ -1,24 +1,33 @@
 import React from "react";
 import PropTypes from "prop-types";
+import { transparentize } from "polished";


designmatty · 2019-07-29T14:41:32Z

packages/core/components/dialog/index.js

+  minWidth: ${themeGet("dialog.minWidths.minWidth")};
+  maxWidth: ${themeGet("dialog.maxWidths.maxWidth")};
+  minHeight: ${themeGet("dialog.minHeights.minHeight")};
+  maxHeight: ${themeGet("dialog.maxHeights.maxHeight")};


incorrect syntax

Gah! I keep doing that.

designmatty · 2019-07-29T14:48:10Z

packages/core/components/dialog/index.js

@@ -70,7 +79,7 @@ const Dialog = ({
    display: 'flex',
    justifyContent: 'center',
    alignItems: 'center',
-    backgroundColor: `rgba(0, 0, 0, ${themeGet(`opacity.${opacity}`, opacity)(props)})`,
+    backgroundColor: `rgba(0, 0, 0, ${opacity})`,


we might consider allowing user-defined colors here.

I tried that – it's actually the source of that stray polished transparentize import – but couldn't get it working. Decided to punt for now. I'll open an issue.

The following should work:

const overlayStyle = { ... background: transparentize(opacity, backdropColor) }

And obviously, you would then need to pass backdropColor as a prop and pass a defaultProp value.

The biggest downside to this approach is the lack of access to the theme. That might require a larger rewrite.

Yeah the problem I had was supporting the overlay color in the theme file too. Would have to add some logic to use the prop if present, else fallback on the theme. We do this a lot inside our styled component definitions, but not in a standalone const. I'm sure it's doable, I just hit a wall and so punted.

Okay, yeah that's fair. Agreed on the issue then.

designmatty · 2019-07-29T15:00:24Z

packages/core/components/dialog/index.mdx

-Notable props include `opacity` for adjusting the scrim opacity, `boxShadow` for
-setting the dialog drop shadow, and `closeTimeoutMS` for adjusting the transition
-timing. Transitions are hard-coded for now. Only the timing is adjustable.
+Overlay opacity can be adjusted via the `opacity` prop.


If you end up making the suggested change about user-defined colors, update this as well.

designmatty · 2019-07-29T16:59:57Z

packages/core/components/button/index.js

@@ -49,7 +45,7 @@ const ButtonLoading = styled.span`
  align-items: center;
  justify-content: center;

-  > ${ButtonIcon} {
+  > .icon {


Nothing wrong with this change, just curious about the decision behind it.

ButtonIcon was a styled(Icon). I've opted to stop using that syntax entirely (in favor of styled.div or other built-in html elements), so I just used the Icon component directly in the jsx. Hence needed to give it a classname to refer to it here.

designmatty · 2019-07-29T17:01:18Z

packages/core/components/dialog/index.mdx

@@ -7,47 +7,89 @@ route: /overlays/dialog
 import { Playground, Props } from 'docz'
 import Dialog from './'
 import Button from '../../../core/components/button'
+import Heading from '../../../core/components/heading'


these imports can simply be '../button' and '../heading'

designmatty · 2019-07-29T17:19:40Z

packages/core/components/tooltip/index.js

@@ -1,38 +1,43 @@
 import React from "react";
 import PropTypes from "prop-types";
 import styled, { css } from "styled-components";


css import not needed

lederer · 2019-07-29T18:20:38Z

Thanks for catching all those. Ready for another look.

designmatty

Looks good

* Add aria-hidden to Callout icon and Breadcrumbs icons * Remove styled(Icon) from Button. Add aria attributes. * Update Tooltip to new theme system * Update Dialog to new theme system * Align Checkbox and Radio * Add justify prop to Grouper

lederer added 4 commits July 25, 2019 16:39

Add aria-hidden to Callout icon and Breadcrumbs icons

06172b8

Remove styled(Icon) from Button. Add aria attributes.

ef0d895

Update Tooltip to new theme system

9fcdff3

Update Dialog to new theme system

6fc6e11

lederer requested a review from designmatty July 26, 2019 18:16

Align Checkbox and Radio

eeec998

Add justify prop to Grouper

26d8058

designmatty suggested changes Jul 29, 2019

View reviewed changes

designmatty reviewed Jul 29, 2019

View reviewed changes

Requested changes

de6343f

designmatty approved these changes Jul 29, 2019

View reviewed changes

lederer merged commit 3a39001 into feature/theme Jul 29, 2019

lederer deleted the feature/sml/theme-overlays branch July 29, 2019 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update overlay components to new theme system #209

Update overlay components to new theme system #209

lederer commented Jul 26, 2019 •

edited

Loading

lederer commented Jul 29, 2019

lederer commented Jul 29, 2019

designmatty Jul 29, 2019

designmatty Jul 29, 2019

lederer Jul 29, 2019

designmatty Jul 29, 2019

lederer Jul 29, 2019

designmatty Jul 29, 2019 •

edited

Loading

designmatty Jul 29, 2019

designmatty Jul 29, 2019

lederer Jul 29, 2019

designmatty Jul 29, 2019

designmatty Jul 29, 2019

designmatty Jul 29, 2019

lederer Jul 29, 2019

designmatty Jul 29, 2019

designmatty Jul 29, 2019

lederer commented Jul 29, 2019

designmatty left a comment

Update overlay components to new theme system #209

Update overlay components to new theme system #209

Conversation

lederer commented Jul 26, 2019 • edited Loading

Target Branch

Overview

Checklist

Upgrade instructions

Notes

Testing Instructions

lederer commented Jul 29, 2019

lederer commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

designmatty Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lederer commented Jul 29, 2019

designmatty left a comment

Choose a reason for hiding this comment

lederer commented Jul 26, 2019 •

edited

Loading

designmatty Jul 29, 2019 •

edited

Loading