Merge pull request #300 from vrk-kpa/fix/language-menu-positioning

[BugFix] Language menu positioning
vrk-kpa · Mar 25, 2020 · 5cf3303 · 5cf3303
2 parents f563016 + 6810235
commit 5cf3303
Show file tree

Hide file tree

Showing 16 changed files with 472 additions and 150 deletions.
diff --git a/package.json b/package.json
@@ -105,8 +105,8 @@
     "typescript": "3.3.1"
   },
   "dependencies": {
-    "@reach/menu-button": "0.8.5",
-    "@reach/popover": "0.8.5",
+    "@reach/menu-button": "0.9.0",
+    "@reach/popover": "0.9.0",
     "classnames": "2.2.6",
     "normalize.cssinjs": "1.1.0",
     "polished": "3.4.0",

diff --git a/src/components/Dropdown/Dropdown.tsx b/src/components/Dropdown/Dropdown.tsx
@@ -6,6 +6,7 @@ import {
   MenuButton,
   MenuButtonProps,
   MenuPopoverProps,
+  MenuItems,
   MenuItem,
   MenuItemProps,
   MenuPopover,
@@ -17,7 +18,7 @@ export { MenuItem as DropdownItem };
 
 const baseClassName = 'fi-dropdown';
 const buttonClassName = `${baseClassName}_button`;
-const dropdownListClassName = `${baseClassName}_list`;
+const dropdownPopoverClassName = `${baseClassName}_popover`;
 const dropdownItemClassName = `${baseClassName}_item`;
 
 export interface DropdownItemProps {
@@ -27,7 +28,7 @@ export interface DropdownItemProps {
   children: ReactNode;
 }
 
-type DropdownListItems = DropdownItemProps;
+type DropdownPopoverItems = DropdownItemProps;
 
 interface DropdownState {
   selectedName: ReactNode;
@@ -36,7 +37,7 @@ interface DropdownState {
 type OptionalMenuButtonProps = {
   [K in keyof MenuButtonProps]?: MenuButtonProps[K];
 };
-type OptionalMenuListProps = {
+type OptionalMenuPopoverProps = {
   [K in keyof MenuPopoverProps]?: MenuPopoverProps[K];
 };
 type OptionalMenuItemProps = { [K in keyof MenuItemProps]?: MenuItemProps[K] };
@@ -52,23 +53,23 @@ export interface DropdownProps {
   className?: string;
   /** Properties given to dropdown's Button-component, className etc. */
   dropdownButtonProps?: OptionalMenuButtonProps;
-  /** Properties given to dropdown's list-component, className etc. */
-  dropdownListProps?: OptionalMenuListProps;
-  menuListComponent?: React.ComponentType<OptionalMenuListProps>;
+  /** Properties given to dropdown's popover-component, className etc. */
+  dropdownPopoverProps?: OptionalMenuPopoverProps;
+  menuPopoverComponent?: React.ComponentType<OptionalMenuPopoverProps>;
   /** Properties given to dropdown's item-component, className etc. */
   dropdownItemProps?: OptionalMenuItemProps;
   /** DropdownItems */
   children?:
-    | Array<ReactElement<DropdownListItems>>
-    | ReactElement<DropdownListItems>;
+    | Array<ReactElement<DropdownPopoverItems>>
+    | ReactElement<DropdownPopoverItems>;
 }
 
 export class Dropdown extends Component<DropdownProps> {
   state: DropdownState = { selectedName: undefined };
 
   changeName = (name: ReactNode) => this.setState({ selectedName: name });
 
-  list = (
+  dropDownItems = (
     children: ReactNode,
     changeNameToSelection: boolean,
     dropdownItemProps?: OptionalMenuItemProps,
@@ -99,8 +100,8 @@ export class Dropdown extends Component<DropdownProps> {
       name,
       className,
       dropdownButtonProps = {},
-      dropdownListProps = {},
-      menuListComponent: MenuListComponentReplace,
+      dropdownPopoverProps = {},
+      menuPopoverComponent: MenuPopoverComponentReplace,
       dropdownItemProps = {},
       changeNameToSelection = true,
       ...passProps
@@ -116,9 +117,12 @@ export class Dropdown extends Component<DropdownProps> {
       ...dropdownButtonProps,
       className: classnames(buttonClassName, dropdownButtonProps.className),
     };
-    const passDropdownListProps = {
-      ...dropdownListProps,
-      className: classnames(dropdownListClassName, dropdownListProps.className),
+    const passDropdownPopoverProps = {
+      ...dropdownPopoverProps,
+      className: classnames(
+        dropdownPopoverClassName,
+        dropdownPopoverProps.className,
+      ),
     };
     const passDropdownItemProps = {
       ...dropdownItemProps,
@@ -131,24 +135,26 @@ export class Dropdown extends Component<DropdownProps> {
           <MenuButton {...passDropdownButtonProps}>
             {!!selectedName ? selectedName : name}
           </MenuButton>
-          {!!MenuListComponentReplace ? (
-            <MenuListComponentReplace {...passDropdownListProps}>
-              {this.list(
+          {!!MenuPopoverComponentReplace ? (
+            <MenuPopoverComponentReplace {...passDropdownPopoverProps}>
+              {this.dropDownItems(
                 children,
                 changeNameToSelection,
                 passDropdownItemProps,
               )}
-            </MenuListComponentReplace>
+            </MenuPopoverComponentReplace>
           ) : (
             <MenuPopover
               position={positionMatchWidth}
-              {...passDropdownListProps}
+              {...passDropdownPopoverProps}
             >
-              {this.list(
-                children,
-                changeNameToSelection,
-                passDropdownItemProps,
-              )}
+              <MenuItems>
+                {this.dropDownItems(
+                  children,
+                  changeNameToSelection,
+                  passDropdownItemProps,
+                )}
+              </MenuItems>
             </MenuPopover>
           )}
         </Menu>

diff --git a/src/components/LanguageMenu/LanguageMenu.tsx b/src/components/LanguageMenu/LanguageMenu.tsx
@@ -4,24 +4,18 @@ import { HtmlSpan } from '../../reset/HtmlSpan/HtmlSpan';
 import {
   Menu,
   MenuButton,
-  MenuList,
-  MenuListProps,
+  MenuItems,
   MenuItem,
   MenuLink,
   MenuPopover,
   MenuPopoverProps,
 } from '@reach/menu-button';
+import { positionDefault } from '@reach/popover';
+import { PRect } from '@reach/rect';
 import { logger } from '../../utils/logger';
 import { Omit } from '../../utils/typescript';
 
-export {
-  MenuList,
-  MenuListProps,
-  MenuItem,
-  MenuLink,
-  MenuPopover,
-  MenuPopoverProps,
-};
+export { MenuItems, MenuItem, MenuLink, MenuPopover, MenuPopoverProps, PRect };
 
 const baseClassName = 'fi-language-menu';
 export interface LanguageMenuItemProps {
@@ -46,11 +40,11 @@ interface LanguageMenuLinkPropsWithType {
 export interface LanguageMenuLinkProps
   extends Omit<LanguageMenuLinkPropsWithType, 'type'> {}
 
-export type LanguageMenuListItemsProps =
+export type LanguageMenuPopoverItemsProps =
   | LanguageMenuItemProps
   | LanguageMenuLinkPropsWithType;
-type OptionalLanguageMenuListProps = {
-  [K in keyof MenuListProps]?: MenuListProps[K];
+type OptionalLanguageMenuPopoverProps = {
+  [K in keyof MenuPopoverProps]?: MenuPopoverProps[K];
 };
 
 export interface LanguageMenuProps {
@@ -62,13 +56,13 @@ export interface LanguageMenuProps {
   languageMenuButtonClassName?: string;
   /** Custom classname to apply when menu is open */
   languageMenuOpenButtonClassName?: string;
-  /** Properties given to LanguageMenu's List-component, className etc. */
-  languageMenuListProps?: OptionalLanguageMenuListProps;
-  languageMenuListComponent?: React.ComponentType<
-    OptionalLanguageMenuListProps
+  /** Properties given to LanguageMenu's popover-component, className etc. */
+  languageMenuPopoverProps?: OptionalLanguageMenuPopoverProps;
+  languageMenuPopoverComponent?: React.ComponentType<
+    OptionalLanguageMenuPopoverProps
   >;
   /** Menu items: MenuItem or MenuLink */
-  children?: Array<React.ReactElement<LanguageMenuListItemsProps>>;
+  children?: Array<React.ReactElement<LanguageMenuPopoverItemsProps>>;
 }
 
 export class LanguageMenu extends Component<LanguageMenuProps> {
@@ -79,8 +73,8 @@ export class LanguageMenu extends Component<LanguageMenuProps> {
       className,
       languageMenuButtonClassName: menuButtonClassName,
       languageMenuOpenButtonClassName: menuButtonOpenClassName,
-      languageMenuListProps: menuListProps = {},
-      languageMenuListComponent: MenuListComponentReplace,
+      languageMenuPopoverProps: menuPopoverProps = {},
+      languageMenuPopoverComponent: MenuPopoverComponentReplace,
       ...passProps
     } = this.props;
 
@@ -103,12 +97,14 @@ export class LanguageMenu extends Component<LanguageMenuProps> {
                 >
                   {name}
                 </MenuButton>
-                {!!MenuListComponentReplace ? (
-                  <MenuListComponentReplace {...menuListProps}>
+                {!!MenuPopoverComponentReplace ? (
+                  <MenuPopoverComponentReplace {...menuPopoverProps}>
                     {children}
-                  </MenuListComponentReplace>
+                  </MenuPopoverComponentReplace>
                 ) : (
-                  <MenuList {...menuListProps}>{children}</MenuList>
+                  <MenuPopover position={positionDefault} {...menuPopoverProps}>
+                    <MenuItems>{children}</MenuItems>
+                  </MenuPopover>
                 )}
               </Fragment>
             );

diff --git a/src/components/index.ts b/src/components/index.ts
@@ -20,7 +20,7 @@ export { LinkExternal, LinkExternalProps } from './Link/LinkExternal';
 export {
   LanguageMenu,
   LanguageMenuProps,
-  LanguageMenuListItemsProps,
+  LanguageMenuPopoverItemsProps,
   LanguageMenuItemProps,
   LanguageMenuLinkProps as MenuLinkProps,
   MenuItem,

diff --git a/src/core/Dropdown/Dropdown.baseStyles.tsx b/src/core/Dropdown/Dropdown.baseStyles.tsx
@@ -31,9 +31,9 @@ export const baseStyles = withSuomifiTheme(
   `,
 );
 
-export const menuListStyles = withSuomifiTheme(
+export const menuPopoverStyles = withSuomifiTheme(
   ({ theme }: TokensAndTheme) => css`
-  &[data-reach-menu-popover].fi-dropdown_list {
+  &[data-reach-menu-popover].fi-dropdown_popover {
     ${element({ theme })}
     ${theme.typography.actionElementInnerText}
     margin-top: -1px;
@@ -48,6 +48,11 @@ export const menuListStyles = withSuomifiTheme(
     border-radius: 0px 0px ${theme.radius.basic} ${theme.radius.basic};
     overflow: hidden;
   }
+  
+  & [data-reach-menu-items] {
+    border: 0;
+    padding: 0;
+  }
 
   & [data-reach-menu-item].fi-dropdown_item {
     ${element({ theme })}

diff --git a/src/core/Dropdown/Dropdown.test.tsx b/src/core/Dropdown/Dropdown.test.tsx
@@ -1,7 +1,10 @@
 import React from 'react';
-import { axeTest } from '../../utils/test/axe';
+import { render } from '@testing-library/react';
 
 import { Dropdown } from './Dropdown';
+import { baseStyles } from './Dropdown.baseStyles';
+import { cssFromBaseStyles } from '../utils';
+import { axeTest } from '../../utils/test/axe';
 
 const doNothing = () => ({});
 
@@ -12,4 +15,24 @@ const TestDropdown = (
   </Dropdown>
 );
 
-test('should not have basic accessibility issues', axeTest(TestDropdown));
+test('calling render with the same component on the same container does not remount', () => {
+  const { container } = render(TestDropdown);
+  expect(container).toMatchSnapshot();
+});
+
+// Don't validate aria-attributes since Portal is not rendered and there is no pair for aria-controls
+test(
+  'should not have basic accessibility issues',
+  axeTest(TestDropdown, {
+    rules: {
+      'aria-valid-attr-value': {
+        enabled: false,
+      },
+    },
+  }),
+);
+
+test('CSS export', () => {
+  const css = cssFromBaseStyles(baseStyles);
+  expect(css).toEqual(expect.stringContaining('color'));
+});
diff --git a/src/core/Dropdown/Dropdown.tsx b/src/core/Dropdown/Dropdown.tsx
@@ -2,10 +2,11 @@ import React, { Component, Fragment } from 'react';
 import { default as styled } from 'styled-components';
 import { withSuomifiDefaultProps } from '../theme/utils';
 import { TokensProp, InternalTokensProp } from '../theme';
-import { baseStyles, menuListStyles } from './Dropdown.baseStyles';
+import { baseStyles, menuPopoverStyles } from './Dropdown.baseStyles';
 import {
   MenuPopover as CompMenuPopover,
-  MenuListProps as CompMenuListProps,
+  MenuPopoverProps as CompMenuPopoverProps,
+  MenuItems as CompMenuItems,
 } from '../../components/LanguageMenu/LanguageMenu';
 import {
   Dropdown as CompDropdown,
@@ -27,17 +28,21 @@ const StyledDropdown = styled(
   ${props => baseStyles(props)}
 `;
 
-interface MenuListProps extends CompMenuListProps, TokensProp {}
+interface MenuPopoverProps extends CompMenuPopoverProps, TokensProp {}
 
-const StyledMenuList = styled(({ tokens, ...passProps }: MenuListProps) => (
-  <CompMenuPopover position={positionMatchWidth} {...passProps} />
-))`
-  ${props => menuListStyles(props.theme)}
+const StyledMenuPopover = styled(
+  ({ tokens, children, ...passProps }: MenuPopoverProps) => (
+    <CompMenuPopover position={positionMatchWidth} {...passProps}>
+      <CompMenuItems>{children}</CompMenuItems>
+    </CompMenuPopover>
+  ),
+)`
+  ${props => menuPopoverStyles(props.theme)}
 `;
 
 /**
  * <i class="semantics" />
- * Use for selectable dropdown list.
+ * Use for selectable dropdown with items.
  */
 export class Dropdown extends Component<DropdownProps> {
   static item = (props: DropdownItemProps) => <DropdownItem {...props} />;
@@ -46,7 +51,7 @@ export class Dropdown extends Component<DropdownProps> {
     const props = withSuomifiDefaultProps(this.props);
     return (
       <Fragment>
-        <StyledDropdown {...props} menuListComponent={StyledMenuList} />
+        <StyledDropdown {...props} menuPopoverComponent={StyledMenuPopover} />
       </Fragment>
     );
   }