Skip to content

Conversation

eliias
Copy link
Contributor

@eliias eliias commented Feb 4, 2019

WHY are these changes introduced?

This addresses some of the discussions that we had after this issue popped up #956 and is also a follow up of #852. The original PR did change the width of the search input field in the TopBar but did not apply those changes to the search results container. This looks a bit weird and this PR aligns the search result with the input field and also matches the width of the field.

WHAT is this pull request doing?

It sets the position rule to be relative on the search container. This allows us to match the width with the absolutely positioned search results container.

screen shot 2019-02-04 at 4 50 09 pm

screen shot 2019-02-04 at 4 57 10 pm

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import * as React from 'react';
import {ActionList, AppProvider, Card, Frame, Page, TopBar} from '../src';

interface State {}

export default class Playground extends React.Component<{}, State> {
  state = {
    userMenuOpen: false,
    searchActive: false,
    searchText: '',
  };

  toggleUserMenu = () => {
    // @ts-ignore
    this.setState(({userMenuOpen}) => ({userMenuOpen: !userMenuOpen}));
  };

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

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

  render() {
    const {
      state,
      handleSearchChange,
      handleSearchResultsDismiss,
      toggleUserMenu,
    } = this;

    const {userMenuOpen, searchText, searchActive} = state;

    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',
        url: 'http://jadedpixel.com',
        accessibilityLabel: 'Jaded Pixel',
      },
    };

    const userMenuMarkup = (
      <TopBar.UserMenu
        actions={[
          {
            items: [{content: 'Back to Shopify', icon: 'arrowLeft'}],
          },
          {
            items: [{content: 'Community forums'}],
          },
        ]}
        name="Dharma"
        detail="Jaded Pixel"
        initials="D"
        open={userMenuOpen}
        onToggle={toggleUserMenu}
      />
    );

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

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

    const topBarMarkup = (
      <TopBar
        showNavigationToggle={true}
        userMenu={userMenuMarkup}
        searchResultsVisible={searchActive}
        searchField={searchFieldMarkup}
        searchResults={searchResultsMarkup}
        onSearchResultsDismiss={handleSearchResultsDismiss}
        onNavigationToggle={() => {
          console.log('toggle navigation visibility');
        }}
      />
    );

    return (
      <Page>
        <AppProvider theme={theme}>
          <Frame topBar={topBarMarkup} />
        </AppProvider>
      </Page>
    );
  }
}

🎩 checklist

@eliias eliias self-assigned this Feb 4, 2019
@eliias eliias requested a review from kvendrik February 4, 2019 21:58
@ghost
Copy link

ghost commented Feb 4, 2019

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

@ghost ghost added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Feb 4, 2019
@BPScott BPScott temporarily deployed to polaris-react-pr-969 February 4, 2019 21:59 Inactive
@kvendrik kvendrik requested a review from ry5n February 4, 2019 22:23
@ghost ghost removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Feb 4, 2019
@eliias eliias force-pushed the align-search-results branch from b0e8685 to 852e2bb Compare February 4, 2019 22:31
@BPScott BPScott temporarily deployed to polaris-react-pr-969 February 4, 2019 22:32 Inactive
Copy link
Contributor

@ry5n ry5n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for large screens, but for small screens it makes the results overlay too narrow. Previously I think we solved this by making the overlay full-width below a certain breakpoint (we’re still removing the border radius at this breakpoint now).

screen shot 2019-02-04 at 3 00 00 pm

@eliias eliias force-pushed the align-search-results branch from 852e2bb to 45984ae Compare February 5, 2019 20:28
@BPScott BPScott temporarily deployed to polaris-react-pr-969 February 5, 2019 20:28 Inactive
@eliias eliias force-pushed the align-search-results branch from 45984ae to af2a917 Compare February 5, 2019 20:28
@BPScott BPScott temporarily deployed to polaris-react-pr-969 February 5, 2019 20:29 Inactive
@eliias eliias force-pushed the align-search-results branch from af2a917 to eb53401 Compare February 5, 2019 20:33
@BPScott BPScott temporarily deployed to polaris-react-pr-969 February 5, 2019 20:33 Inactive
@eliias eliias force-pushed the align-search-results branch from eb53401 to 6531b03 Compare February 5, 2019 20:35
@BPScott BPScott temporarily deployed to polaris-react-pr-969 February 5, 2019 20:35 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-969 February 5, 2019 20:36 Inactive
@eliias eliias force-pushed the align-search-results branch from c5df99c to 67d203a Compare February 5, 2019 20:36
@BPScott BPScott temporarily deployed to polaris-react-pr-969 February 5, 2019 20:37 Inactive
Copy link
Member

@kvendrik kvendrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure that there are no magic numbers/calculations in there, otherwise this LGTM 👍 Thanks for picking this up @eliias! 💯


@include page-content-when-not-fully-condensed {
align-items: flex-start;
@include breakpoint-after(743px) {
Copy link
Member

@kvendrik kvendrik Feb 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

743px maybe store this in a variable somewhere

align-items: stretch;
width: 100vw;
height: calc(100vh - #{top-bar-height() + spacing(tight)});
top: top-bar-height() - spacing();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might wanna store this (or just the spacing() part) in a variable to create clarity around how this calculation works.

Copy link
Contributor

@ry5n ry5n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels pretty good now. I haven’t been able to properly 🎩 on mobile browsers due to Storybook, but the scaling across screen sizes works well. The results overlay does encroach on the user menu at the right; it doesn’t block it, but there’s a tradeoff here. @jakob-stecher you should take a look at make the final call on the alignment.

@eliias eliias force-pushed the align-search-results branch from 67d203a to baaa662 Compare February 5, 2019 23:06
@BPScott BPScott temporarily deployed to polaris-react-pr-969 February 5, 2019 23:06 Inactive
@eliias
Copy link
Contributor Author

eliias commented Feb 5, 2019

There was still a small issue w/ alignment on mobile. So we took the chance to also reposition the results a bit. Thx for review/polish @kvendrik.

screen shot 2019-02-05 at 6 11 13 pm

screen shot 2019-02-05 at 6 12 27 pm

@ry5n I just used the iframe URL to test on iOS simulator http://localhost:6006/iframe.html?selectedKind=Playground&selectedStory=Playground

@eliias eliias changed the title [WIP][TopBar] adopt search results to match width and alignment of input field [TopBar] adopt search results to match width and alignment of input field Feb 6, 2019
Copy link
Member

@kvendrik kvendrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (one minor comment), Percy changes also make sense to me. If this looks good to @jakob-stecher and @ry5n I say let's 🚢 it.

@include breakpoint-after($large-width) {
width: calc(100% - #{$search-results-desktop-large-width-offset});
}
max-width: 70rem;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe store these numbers somewhere to make them a little less magical.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have extracted both values into variables and got them hoisted to the top of the file.

@eliias eliias force-pushed the align-search-results branch from baaa662 to 58c679a Compare February 6, 2019 20:47
@BPScott BPScott temporarily deployed to polaris-react-pr-969 February 6, 2019 20:47 Inactive
@eliias
Copy link
Contributor Author

eliias commented Feb 7, 2019

Hey folks, we want to get this shipped w/ the next release of Polaris. Please provide your final feedback or please give it a thumbs up. @jakob-stecher @ry5n.

@ghost
Copy link

ghost commented Feb 7, 2019

🎉 Thanks for your contribution to Polaris React!

@eliias eliias deleted the align-search-results branch February 7, 2019 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants