Skip to content

Conversation

AndrewMusgrave
Copy link
Member

WHY are these changes introduced?

Part of #1484

WHAT is this pull request doing?

I'm converting FileUpload to a functional component and removing withRef / withContext

How to 🎩

Tests / percy / development server

@BPScott BPScott temporarily deployed to polaris-react-pr-1491 May 15, 2019 18:33 Inactive
import DropZoneContext from '../../context';
import {usePolaris} from '../../../../hooks';

export interface State {
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 have no need for state. See below 👀

const {size, type} = React.useContext(DropZoneContext);
const suffix = capitalize(type);
const {
actionTitle = translate(`Polaris.DropZone.FileUpload.actionTitle${suffix}`),
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 use the provided actionTitle/Hint and default it to what was previously the default on state

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why we had done that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like it was to avoid string translation however object lookup is inexpensive so I'm not sure why, or this approach didn't occur when developing the component? 🤷‍♂

@AndrewMusgrave AndrewMusgrave requested a review from dleroux May 15, 2019 18:48
Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

The code looks fine. Just questioning why we were using state to hold those props?

@AndrewMusgrave AndrewMusgrave merged commit 9c86d68 into version-4.0.0 May 21, 2019
@AndrewMusgrave AndrewMusgrave deleted the fu-dep-hoc branch May 21, 2019 15:15
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.

3 participants