Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v4] Add usePolaris/AppBridge #1482

Merged
merged 1 commit into from
May 15, 2019

Conversation

AndrewMusgrave
Copy link
Member

WHY are these changes introduced?

Preparing for hooks

WHAT is this pull request doing?

  • create a new directory for hooks src/hooks
  • create usePolaris hook for internal use
  • create useAppBridge hook for external use
  • 馃帀 tests 馃帀

馃帺

Trust in the powers of tests or try the playground - you'll need credentials for testing useAppBridge

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

interface State {}

const credentials = {
  apiKey: undefined,
  shopOrigin: undefined,
}

export default function() {
  const {apiKey, shopOrigin} = credentials;
  return (
    <AppProvider apiKey={apiKey} shopOrigin={shopOrigin}>
      <Page title="Playground">
        <Card title="usePolaris">
          {JSON.stringify(usePolaris(), null, 2)}
        </Card>

        <Card title="useAppBridge">
          {!apiKey && !shopOrigin ? "You'll need real credentials or this is undefined :shocked:" : '' }
          {JSON.stringify(useAppBridge())}
        </Card>
      </Page>
    </AppProvider>
  );
}

@@ -9,6 +9,7 @@ export {
default as AppProvider,
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to use these in src/hooks and we can't dive into component folders from outside /components

@@ -0,0 +1,10 @@
import * as React from 'react';
Copy link
Member Author

Choose a reason for hiding this comment

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

We'll want this to expose appBridge rather than exporting usePolaris

}

mountWithAppProvider(<Component />);
expect(JSON.stringify(context)).toEqual(
Copy link
Member Author

Choose a reason for hiding this comment

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

Feel odd about this test. But we've done it before and I can't think of a better alternative.

@AndrewMusgrave AndrewMusgrave added the 馃Skip Changelog Causes CI to ignore changelog update check. label May 15, 2019
@AndrewMusgrave AndrewMusgrave merged commit a176047 into appprovider-new-context May 15, 2019
@AndrewMusgrave AndrewMusgrave deleted the create-polaris-hooks branch May 15, 2019 16:21
@BPScott
Copy link
Member

BPScott commented May 16, 2019

I'm a little late to the party on this one but I've got a thought:

Hooks should be pretty small and self contained - each hook would never sprawl across multiple files (unlike components that may have partner scss files and things like that). As such I don't think there's any value in creating a "folder per hook" hierarchy as there's only ever going to be one file (+ the test) for each hook within it.

With that in mind I don't think there's any value in having src/hooks/use-polaris/use-polaris.tsx it adds depth for no value, I think we should just put hooks directly in the hooks folder e.g.: src/hooks/use-polaris.ts and its test in src/hooks/tests/use-polaris.ts.

This was referenced May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
馃Skip Changelog Causes CI to ignore changelog update check.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants