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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hookify more components #2083

Merged
merged 2 commits into from
Sep 11, 2019
Merged

Hookify more components #2083

merged 2 commits into from
Sep 11, 2019

Conversation

BPScott
Copy link
Member

@BPScott BPScott commented Sep 6, 2019

As with other hookify PRs this is probably best viewed with supresed whitespace.

WHY are these changes introduced?

More work towards #1995.

WHAT is this pull request doing?

Hookifies RangeSlider, ScrollLock and Topbar's SearchField

How to 🎩

  • Confirm Single / Dual rangeslider examples work
  • Confirm Topbar SearchField works, by checking the Frame example (The TopBar example seems to be buggy losing content on blur on master)
  • Confirm ScrollLock works by checking modal overlays stop you scrolling the page within web. ScrollLock doesn't work within storybook even on master for whatever reason.

Copy link
Member

@AndrewMusgrave AndrewMusgrave left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@@ -2,10 +2,7 @@ import React from 'react';
import {CircleCancelMinor} from '@shopify/polaris-icons';
import {ReactWrapper} from 'enzyme';
import {mountWithAppProvider} from 'test-utilities/legacy';
// We've got to export a named SearchField otherwise other stuff breaks
Copy link
Member

Choose a reason for hiding this comment

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

Just curious what this was breaking?

Copy link
Member Author

Choose a reason for hiding this comment

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

It complained about being able to generate the types for TopBar.SearchField - or rather it was ok generating them, but trying to use it in consuming projects caused complaints. exporting SearchField in addition to the default component seemed to make it happy.

I don't understand the reasoning for it, but I seemed like it was a due to a combo of a trying to use a withAppProvider wrapped component as an exposed subcomponent


interface ScrollLockProps {}

// Even though this has no args, reference ScrollLockProps so the prop explorer
Copy link
Member

Choose a reason for hiding this comment

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

Will this show up in the style guide without a Props interface?

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, looks like we don't have a readme and this is just an internal component.

Copy link
Member Author

@BPScott BPScott Sep 11, 2019

Choose a reason for hiding this comment

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

Per my proposal in #1959 we now name Props after the component - see ScrollLockProps above this. This stops us having to rename the interface when reexporting it in src/components/index.ts and src/components/ScrollLock/index.ts

The styleguide was updated to handle this in https://github.com/Shopify/polaris-styleguide/pull/2932

src/components/ScrollLock/ScrollLock.ts Show resolved Hide resolved
src/components/ScrollLock/ScrollLock.ts Show resolved Hide resolved
}, [onCancel, onChange]);

useEffect(() => {
if (input.current) {
Copy link
Member

Choose a reason for hiding this comment

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

Small nitpick: should we have an early return so the entire effect isn't wrapped in a block?

if (!input.current) return;
focused ? input.current.focus() : input.current.blur();

Copy link
Member Author

Choose a reason for hiding this comment

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

good call

- RangeSlider
- ScrollLock
- Topbar's SearchField
@BPScott BPScott merged commit ec0ea73 into master Sep 11, 2019
@BPScott BPScott deleted the more-hook branch September 11, 2019 22:24
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.

2 participants