Repository: uber-web/baseui
Pull request #1012 (2019-03-12 19:49:29)

docs: add notes about longhand vs shorthand props
There was some confusion about shorthand vs longhand CSS properties so I've added some description into the Understanding Overrides page and overrides inspector.


Patch:
diff --git a/documentation-site/components/overrides.js b/documentation-site/components/overrides.js
index af7a62461..70041bdab 100644
--- a/documentation-site/components/overrides.js
+++ b/documentation-site/components/overrides.js
@@ -84,6 +84,14 @@ class Overrides extends React.Component {
             },
           })}
         </Block>
+        <Block as="p" font="font400" marginTop="scale900">
+          <b>Note:</b> You should always use longhand CSS properties. Mixing
+          shorthands and longhands will lead into{' '}
+          <DocLink href="https://www.styletron.org/concepts/#shorthand-and-longhand-properties">
+            strange behaviors
+          </DocLink>
+          !
+        </Block>
       </React.Fragment>
     );
   }
diff --git a/documentation-site/pages/theming/understanding-overrides.mdx b/documentation-site/pages/theming/understanding-overrides.mdx
index 89b57c630..c1f603285 100644
--- a/documentation-site/pages/theming/understanding-overrides.mdx
+++ b/documentation-site/pages/theming/understanding-overrides.mdx
@@ -83,6 +83,8 @@ We defined `overrides.Label.style` and `overrides.Label.props` properties and th
 
 The `overrides.Label.style` property accepts a [style object](https://www.styletron.org/concepts/#style-object) or [style function](https://www.styletron.org/concepts/#style-function) since [Styletron](https://www.styletron.org) manages all Base Web styles.
 
+> **Caveat:** When using `overrides.foo.style`, you are overriding a set of existing CSS properties. **Our components always use longhand CSS properties and so should yours!** If you [mix shorthand and longhand properties](https://www.styletron.org/concepts/#shorthand-and-longhand-properties), you will see a warning and can run into strange behaviors!
+
 ## `$theme`
 
 If you opt-in for the [style function](https://www.styletron.org/concepts/#style-function), `overrides` provides a special prop called **`$theme`** that you can use. The `$theme` prop includes all Base Web [design constants](/theming/theming-values). So instead of the hard-coded value `#892C21`, you can use the theme:
diff --git a/src/datepicker/datepicker.js b/src/datepicker/datepicker.js
index 2d5f934c7..d4b712348 100644
--- a/src/datepicker/datepicker.js
+++ b/src/datepicker/datepicker.js
@@ -148,7 +148,25 @@ export default class Datepicker extends React.Component<
               mountNode={this.props.mountNode}
               placement={PLACEMENT.bottom}
               isOpen={this.state.isOpen}
-              onClickOutside={this.close}
+              onClickOutside={event => {
+                // Required to check that items rendered in a sub-popover does not trigger close.
+                // For example, upon selecting an option from the month dropdown it would cause
+                // this code to run since the two popovers are DOM siblings rather than parent/child.
+                // There's likely a more robust way to check this, but ignores clicks from elements
+                // that are select options for now.
+                function isOption(element) {
+                  if (!element) return false;
+                  return element.getAttribute('role') === 'option';
+                }
+                if (
+                  isOption(event.target) ||
+                  isOption(event.target.parentElement)
+                ) {
+                  return;
+                }
+
+                this.close();
+              }}
               onEsc={this.handleEsc}
               content={
                 <Calendar
diff --git a/src/popover/popover.js b/src/popover/popover.js
index dcaad4a5a..ff1c84c98 100644
--- a/src/popover/popover.js
+++ b/src/popover/popover.js
@@ -276,14 +276,14 @@ class Popover extends React.Component<PopoverPropsT, PopoverPrivateStateT> {
 
   addDomEvents() {
     if (__BROWSER__) {
-      document.addEventListener('mousedown', this.onDocumentClick);
+      document.addEventListener('click', this.onDocumentClick);
       document.addEventListener('keyup', this.onKeyPress);
     }
   }
 
   removeDomEvents() {
     if (__BROWSER__) {
-      document.removeEventListener('mousedown', this.onDocumentClick);
+      document.removeEventListener('click', this.onDocumentClick);
       document.removeEventListener('keyup', this.onKeyPress);
     }
   }
diff --git a/src/select/dropdown.js b/src/select/dropdown.js
index ae30e9e05..98ead95f0 100644
--- a/src/select/dropdown.js
+++ b/src/select/dropdown.js
@@ -134,7 +134,7 @@ export default class SelectDropdown extends React.Component<DropdownPropsT> {
                   overrides: {
                     ListItem: {
                       component: ListItem,
-                      props: listItemProps,
+                      props: {...listItemProps, role: 'option'},
                       // slightly a hacky way to handle the list item style overrides
                       // since the menu component doesn't provide a top level overrides for it
                       // $FlowFixMe


Test patch:
diff --git a/src/datepicker/__tests__/datepicker.e2e.js b/src/datepicker/__tests__/datepicker.e2e.js
index af6dee09e..e400cc2c1 100644
--- a/src/datepicker/__tests__/datepicker.e2e.js
+++ b/src/datepicker/__tests__/datepicker.e2e.js
@@ -22,7 +22,9 @@ const selectors = {
   leftArrow: '[aria-label="Previous month"]',
   rightArrow: '[aria-label="Next month"]',
   monthSelect: '[data-id="monthSelect"]',
+  monthDropdown: '[data-id="monthDropdown"]',
   yearSelect: '[data-id="yearSelect"]',
+  yearDropdown: '[data-id="yearDropdown"]',
   selectDropdown: 'ul[role="listbox"]',
 };
 
@@ -155,7 +157,7 @@ describe('Datepicker', () => {
     await page.waitFor(selectors.calendar);
     await page.waitFor(selectors.day);
     await page.click(selectors.yearSelect);
-    await page.waitFor(selectors.selectDropdown);
+    await page.waitFor(selectors.yearDropdown);
 
     await page.$$eval('ul[role="listbox"] li', items => {
       const option = items.find(item => {
@@ -165,6 +167,7 @@ describe('Datepicker', () => {
       return option;
     });
 
+    await page.waitFor(selectors.yearDropdown, {hidden: true});
     await page.waitFor(selectors.calendar);
     await page.waitFor(selectors.day5);
   });
@@ -176,7 +179,7 @@ describe('Datepicker', () => {
     await page.waitFor(selectors.calendar);
     await page.waitFor(selectors.day);
     await page.click(selectors.monthSelect);
-    await page.waitFor(selectors.selectDropdown);
+    await page.waitFor(selectors.monthDropdown);
 
     await page.$$eval('ul[role="listbox"] li', items => {
       const option = items.find(item => {
@@ -186,6 +189,7 @@ describe('Datepicker', () => {
       return option;
     });
 
+    await page.waitFor(selectors.monthDropdown, {hidden: true});
     await page.waitFor(selectors.calendar);
     await page.waitFor(selectors.day6);
   });
diff --git a/src/datepicker/__tests__/datepicker.scenario.js b/src/datepicker/__tests__/datepicker.scenario.js
index 99599ec49..0ad5abdd5 100644
--- a/src/datepicker/__tests__/datepicker.scenario.js
+++ b/src/datepicker/__tests__/datepicker.scenario.js
@@ -18,10 +18,20 @@ export const component = () => (
     highlightedDate={new Date('March 10, 2019')}
     overrides={{
       MonthSelect: {
-        props: {overrides: {Root: {props: {'data-id': 'monthSelect'}}}},
+        props: {
+          overrides: {
+            Root: {props: {'data-id': 'monthSelect'}},
+            Dropdown: {props: {'data-id': 'monthDropdown'}},
+          },
+        },
       },
       YearSelect: {
-        props: {overrides: {Root: {props: {'data-id': 'yearSelect'}}}},
+        props: {
+          overrides: {
+            Root: {props: {'data-id': 'yearSelect'}},
+            Dropdown: {props: {'data-id': 'yearDropdown'}},
+          },
+        },
       },
     }}
   />
diff --git a/src/popover/__tests__/popover.test.js b/src/popover/__tests__/popover.test.js
index 8178dc38c..2798f1cf1 100644
--- a/src/popover/__tests__/popover.test.js
+++ b/src/popover/__tests__/popover.test.js
@@ -178,7 +178,7 @@ describe('Popover', () => {
     );
 
     const calls = document.addEventListener.mock.calls;
-    expect(calls[0][0]).toBe('mousedown');
+    expect(calls[0][0]).toBe('click');
     expect(calls[1][0]).toBe('keyup');
 
     calls[1][1]({
diff --git a/src/select/__tests__/__snapshots__/dropdown.test.js.snap b/src/select/__tests__/__snapshots__/dropdown.test.js.snap
index 3e4d9f027..d29304b6e 100644
--- a/src/select/__tests__/__snapshots__/dropdown.test.js.snap
+++ b/src/select/__tests__/__snapshots__/dropdown.test.js.snap
@@ -58,7 +58,9 @@ Object {
       "overrides": Object {
         "ListItem": Object {
           "component": [MockFunction],
-          "props": Object {},
+          "props": Object {
+            "role": "option",
+          },
           "style": undefined,
         },
       },