Skip to content

Conversation

kvendrik
Copy link
Member

@kvendrik kvendrik commented Feb 4, 2019

WHY are these changes introduced?

As part of the context switching updates (details) this PR proposes a contextControl prop thats is added to the <TopBar /> and <Navigation /> components and accepts a component that is ideally used to provide a way for users to switch between admin contexts.

WHAT is this pull request doing?

Adds the contextControl prop to <TopBar /> and <Navigation />.

How to 🎩

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

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    const topBar = (
      <TopBar
        contextControl={
          <TopBar.Menu
            action={[]}
            activatorContent="Click here to switch from one context to another"
          />
        }
      />
    );

    const nav = (
      <Navigation
        location="/"
        contextControl={
          <Button>Click here to switch from one context to another</Button>
        }
      />
    );

    return (
      <AppProvider
        theme={{
          logo: {
            width: 124,
            topBarSource:
              'https://cdn.shopify.com/s/files/1/0446/6937/files/jaded-pixel-logo-color.svg?6215648040070010999',
          },
        }}
      >
        <Frame topBar={topBar} navigation={nav} showMobileNavigation />
      </AppProvider>
    );
  }
}
  1. Run yarn dev and navigate to http://localhost:6006/iframe.html?selectedKind=Playground&selectedStory=Playground.
  2. Make sure the <TopBar.Menu /> shows up in the far left corner of the top bar.
  3. Remove the contextControl prop from the top bar.
  4. Make sure the logo shows up and looks/works as you would expect.
  5. Make sure the Button that was passed into Navigation doesn't show up on the page.
  6. Resize the screen to a mobile size.
  7. Make sure the Button that was passed into Navigation shows up in the side nav.

@BPScott BPScott temporarily deployed to polaris-react-pr-966 February 4, 2019 20:25 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-966 February 4, 2019 20:27 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-966 February 4, 2019 20:45 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-966 February 4, 2019 20:49 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-966 February 4, 2019 20:53 Inactive
Copy link

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

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

Having some issues with the 🎩 instructions. The playground code has some TS linting errors around Topbar.Menu missing required props. Also the playground itself errors out with React.Children.only expected to receive a single React element child.

Could you fix these up and then ping me again for review?

@kvendrik
Copy link
Member Author

kvendrik commented Feb 5, 2019

@danrosenthal I pasted in the Playground code from the description and ran yarn dev and it seems to work well for me. The TopBar.Menu is missing props but none of them are critical for the playground to render. Could you perhaps try again? Feel free to ping me on Slack if it doesn't work.

</UnstyledLink>
) : null;
const contextMarkup =
!contextControl && logo ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

We will want to use if-else flow control here:

if context control -> render context control
else if logo -> render logo
else -> null

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense that makes it a bit clearer and resolves the no contextControl nor logo case.

Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

We are missing a test case for top bar without context control and without logo. Currently that code path with still render and empty context control div.

@kvendrik
Copy link
Member Author

kvendrik commented Feb 6, 2019

We are missing a test case for top bar without context control and without logo. Currently that code path with still render and empty context control div.

Yes good catch! Will fix that!

@BPScott BPScott temporarily deployed to polaris-react-pr-966 February 6, 2019 16:58 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-966 February 6, 2019 16:59 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-966 February 6, 2019 17:00 Inactive
Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

Didn’t 🎩 but code LGTM

@danrosenthal
Copy link

danrosenthal commented Feb 11, 2019

During my 🎩 I noticed this was able to be gotten into a weird state if the initial render was on desktop and you resize it to mobile, where the Backdrop was visible but not the Navigation. When initial render is on mobile, the Navigation renders as expected. This was pretty weird, but I don't see how it could be related to these changes.

(I think it is related to how CSSTransition handles being open before it is allowed to finish its lifecycle. Something we should look into as a followup @tmlayton)

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