[Navigation.Item] Fix onClick not being called on small screens

[AndrewMusgrave]

WHY are these changes introduced?

Fixes #598

WHAT is this pull request doing?

When on a small screens and onNavigationDismiss is undefined onClick is never called

How to 🎩

Make sure onClick is being called when on small screens

• add tests

Copy-paste this code in playground/Playground.tsx:

import * as React from 'react';
import {
  Page,
  Navigation,
  TopBar,
  Frame,
  Toast,
  Loading,
  Card,
  Layout,
  FormLayout,
  SkeletonPage,
  TextContainer,
  SkeletonBodyText,
  SkeletonDisplayText,
  Modal,
  TextField,
  AppProvider,
  ActionList,
} from '@shopify/polaris';

interface State {}

export default class FrameExample extends React.Component {
  defaultState = {
    emailFieldValue: 'dharma@jadedpixel.com',
    nameFieldValue: 'Jaded Pixel',
  };

  state = {
    showToast: false,
    isLoading: false,
    isDirty: false,
    searchActive: false,
    searchText: '',
    userMenuOpen: false,
    showMobileNavigation: false,
    modalActive: false,
    nameFieldValue: this.defaultState.nameFieldValue,
    emailFieldValue: this.defaultState.emailFieldValue,
    storeName: this.defaultState.nameFieldValue,
    supportSubject: '',
    supportMessage: '',
  };

  render() {
    const {
      showToast,
      isLoading,
      isDirty,
      searchActive,
      searchText,
      userMenuOpen,
      showMobileNavigation,
      nameFieldValue,
      emailFieldValue,
      modalActive,
      storeName,
    } = this.state;

    const toastMarkup = showToast ? (
      <Toast
        onDismiss={this.toggleState('showToast')}
        content="Changes saved"
      />
    ) : null;

    const userMenuActions = [
      {
        items: [{content: 'Back to Shopify', icon: 'arrowLeft'}],
      },
      {
        items: [{content: 'Community forums'}],
      },
    ];

    const navigationUserMenuMarkup = (
      <Navigation.UserMenu
        actions={userMenuActions}
        name="Dharma"
        detail={storeName}
        avatarInitials="D"
      />
    );

    const contextualSaveBarMarkup = isDirty ? (
      <ContextualSaveBar
        message="Unsaved changes"
        saveAction={{
          onAction: this.handleSave,
        }}
        discardAction={{
          onAction: this.handleDiscard,
        }}
      />
    ) : null;

    const userMenuMarkup = (
      <TopBar.UserMenu
        actions={userMenuActions}
        name="Dharma"
        detail={storeName}
        initials="D"
        open={userMenuOpen}
        onToggle={this.toggleState('userMenuOpen')}
      />
    );

    const searchResultsMarkup = (
      <Card>
        <ActionList
          items={[
            {content: 'Shopify help center'},
            {content: 'Community forums'},
          ]}
        />
      </Card>
    );

    const searchFieldMarkup = (
      <TopBar.SearchField
        onChange={this.handleSearchFieldChange}
        value={searchText}
        placeholder="Search"
      />
    );

    const topBarMarkup = (
      <TopBar
        showNavigationToggle
        userMenu={userMenuMarkup}
        searchResultsVisible={searchActive}
        searchField={searchFieldMarkup}
        searchResults={searchResultsMarkup}
        onSearchResultsDismiss={this.handleSearchResultsDismiss}
        onNavigationToggle={this.toggleState('showMobileNavigation')}
      />
    );

    const navigationMarkup = (
      <Navigation location="/" userMenu={navigationUserMenuMarkup}>
        <Navigation.Section
          title="Settings"
          items={[
            {
              label: 'Account',
              icon: 'home',
              onClick: this.toggleState('isLoading'),
              url: '/',
            },
            {
              label: 'Orders',
              icon: 'orders',
              onClick: this.toggleState('isLoading'),
            },
          ]}
        />
        <Navigation.Section
          title="Support"
          items={[
            {
              label: 'Help center',
              icon: 'help',
              onClick: this.toggleState('isLoading'),
            },
          ]}
          separator
          action={{
            icon: 'conversation',
            accessibilityLabel: 'Contact support',
            onClick: this.toggleState('modalActive'),
          }}
        />
      </Navigation>
    );

    const loadingMarkup = isLoading ? <Loading /> : null;

    const actualPageMarkup = (
      <Page title="Account">
        <Layout>
          <Layout.AnnotatedSection
            title="Billing details"
            description="We will use this as your billing information."
          >
            <Card sectioned>
              <FormLayout>
                <TextField
                  label="Full name"
                  value={nameFieldValue}
                  onChange={this.handleNameFieldChange}
                />
                <TextField
                  type="email"
                  label="Email"
                  value={emailFieldValue}
                  onChange={this.handleEmailFieldChange}
                />
              </FormLayout>
            </Card>
          </Layout.AnnotatedSection>
        </Layout>
      </Page>
    );

    const loadingPageMarkup = (
      <SkeletonPage>
        <Layout>
          <Layout.Section>
            <Card sectioned>
              <TextContainer>
                <SkeletonDisplayText size="small" />
                <SkeletonBodyText lines={9} />
              </TextContainer>
            </Card>
          </Layout.Section>
        </Layout>
      </SkeletonPage>
    );

    const pageMarkup = isLoading ? loadingPageMarkup : actualPageMarkup;

    const modalMarkup = (
      <Modal
        open={modalActive}
        onClose={this.toggleState('modalActive')}
        title="Contact support"
        primaryAction={{
          content: 'Send',
          onAction: this.toggleState('modalActive'),
        }}
      >
        <Modal.Section>
          <FormLayout>
            <TextField
              label="Subject"
              value={this.state.supportSubject}
              onChange={this.handleSubjectChange}
            />
            <TextField
              label="Message"
              value={this.state.supportMessage}
              onChange={this.handleMessageChange}
              multiline
            />
          </FormLayout>
        </Modal.Section>
      </Modal>
    );

    const theme = {
      colors: {
        topBar: {
          background: '#357997',
        },
      },
      logo: {
        width: 124,
        topBarSource:
          'https://cdn.shopify.com/s/files/1/0446/6937/files/jaded-pixel-logo-color.svg?6215648040070010999',
        contextualSaveBarSource:
          'https://cdn.shopify.com/s/files/1/0446/6937/files/jaded-pixel-logo-gray.svg?6215648040070010999',
        url: 'http://jadedpixel.com',
        accessibilityLabel: 'Jaded Pixel',
      },
    };

    return (
      <div style={{height: '500px'}}>
        <AppProvider theme={theme}>
          <Page>
            <Frame
              topBar={topBarMarkup}
              navigation={navigationMarkup}
              showMobileNavigation={showMobileNavigation}
              onNavigationDismiss={this.toggleState('showMobileNavigation')}
            >
              {contextualSaveBarMarkup}
              {loadingMarkup}
              {pageMarkup}
              {toastMarkup}
              {modalMarkup}
            </Frame>
          </Page>
        </AppProvider>
      </div>
    );
  }

  toggleState = (key) => {
    return () => {
      this.setState((prevState) => ({[key]: !prevState[key]}));
    };
  };

  handleSearchFieldChange = (value) => {
    this.setState({searchText: value});
    if (value.length > 0) {
      this.setState({searchActive: true});
    } else {
      this.setState({searchActive: false});
    }
  };

  handleSearchResultsDismiss = () => {
    this.setState(() => {
      return {
        searchActive: false,
        searchText: '',
      };
    });
  };

  handleEmailFieldChange = (emailFieldValue) => {
    this.setState({emailFieldValue});
    if (emailFieldValue != '') {
      this.setState({isDirty: true});
    }
  };

  handleNameFieldChange = (nameFieldValue) => {
    this.setState({nameFieldValue});
    if (nameFieldValue != '') {
      this.setState({isDirty: true});
    }
  };

  handleSave = () => {
    this.defaultState.nameFieldValue = this.state.nameFieldValue;
    this.defaultState.emailFieldValue = this.state.emailFieldValue;

    this.setState({
      isDirty: false,
      showToast: true,
      storeName: this.defaultState.nameFieldValue,
    });
  };

  handleDiscard = () => {
    this.setState({
      emailFieldValue: this.defaultState.emailFieldValue,
      nameFieldValue: this.defaultState.nameFieldValue,
      isDirty: false,
    });
  };

  handleSubjectChange = (supportSubject) => {
    this.setState({supportSubject});
  };

  handleMessageChange = (supportMessage) => {
    this.setState({supportMessage});
  };
}

[dleroux]

🎩 looks good.

side note: is it me or it's a little tricky to wrap your head around what is really going on here? I understand whats happening but I had to really follow it through.

[danrosenthal]

🎩 LGTM too, though I am similarly confused as to the nature of this issue.

Would you mind annotating your test additions to help others understand how you're simulating a small screen size?

[AndrewMusgrave]

matchMedia is a window function used to match media queries in javascript. Item uses it in ->

polaris-react/src/components/Navigation/components/Item/Item.tsx

Line 301 in 3ab258a

if (onClick && !navigationBarCollapsed().matches) {

to determine screen size

[AndrewMusgrave]

Always returning a screen match

[AndrewMusgrave]

Paired with the utilities navigationBarCollapsed(), it'll always be true.

[BPScott]

I recall @beefchimi being annoyed at matchmedia not being supported by jsdom recently.

I wonder if creating a standard mock implementation in jest-dom-mocks would be useful for everybody.

[RishabhTayal]

@AndrewMusgrave when is this planned to release? Can we use it before the release?

[AndrewMusgrave]

I recall @beefchimi being annoyed at matchmedia not being supported by jsdom recently. I wonder if creating a standard mock implementation in jest-dom-mocks would be useful for everybody.

Great idea!

@RishabhTayal We're planning on doing a patch release ~2pm EST

[RishabhTayal]

@AndrewMusgrave I tested this in 3.0.1 version. The onClick is getting called now, but the navigation is not getting dismissed. Do we have to dismiss it manually? If yes, how can we dismiss on click?

[AndrewMusgrave]

You'll have to dismiss it manually. This is what's used to determine is the mobile navigation is open or closed.

So your onClick would have to change the your local state, it would look something like this.

handleNavigationClick = () => {
  /* Some side effect */
  this.setState(({showMobileNavigation}) => ({showMobileNavigation: !showMobileNavigation}))
  // or this.setState({showMobileNavigation: false})
  ...
}

[brydonm]

I'm pretty sure this is still an issue with nested subNavigationItems. @AndrewMusgrave

              items={[
                {
                  label: "Crafting",
                  url: "/skills/crafting/engineering/pathway",
                  subNavigationItems: [
                    {
                      url: "/skills/crafting/engineering/pathway",
                      label: "Engineering",
                      subNavigationItems: [
                        {
                          url: "/skills/crafting/engineering/pathway",
                          label: "Pathway",
                          onClick: handleNavClick,
                        },
                        {
                          url: "/skills/crafting/engineering/items",
                          label: "Items",
                          onClick: handleNavClick,
                        },
                      ],
                    },
                  ],
                },

Using this items prop on Navigation.Section, you cannot click Pathway or Items on mobile. Crafting and Engineering work fine.

This is on 10.16.1

[AndrewMusgrave]

Hi @brydom I took a look at the types for the navigation component and it seems nested subNavigationItems are not supported. Feel free to open a feature request if this I something you're looking for in the API!