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

Add 'direction' option (prop) to 'withDirection' HOC, This to allow the overwrite of the direction when required #18

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@luis-mgb
Copy link

commented May 11, 2019

Add 'direction' option (prop) to 'withDirection' HOC, This to allow the overwrite of the direction when required (for phone numbers for example that are always LTR), updated packages

@ljharb
Copy link
Member

left a comment

I'm confused about the need for this prop. If you want to force the direction, wouldn't you wrap in a new DirectionProvider, with the direction set explicitly? Why is the prop needed?

Show resolved Hide resolved CHANGELOG.md Outdated
Show resolved Hide resolved CHANGELOG.md Outdated
Show resolved Hide resolved package.json Outdated
Show resolved Hide resolved src/withDirection.jsx
Show resolved Hide resolved tests/AutoDirectionProvider_test.jsx Outdated

@luis-mgb luis-mgb force-pushed the luis-mgb:master branch from e35afdc to 3d3f5c3 May 13, 2019

@ljharb
Copy link
Member

left a comment

Still curious about the above question:

If you want to force the direction, wouldn't you wrap in a new DirectionProvider, with the direction set explicitly? Why is the prop needed?

@@ -23,16 +23,19 @@ const defaultDirection = DIRECTIONS.LTR;

// export for convenience, in order for components to spread these onto their propTypes
export const withDirectionPropTypes = {
direction: directionPropType.isRequired,
direction: directionPropType,

This comment has been minimized.

Copy link
@ljharb

ljharb May 13, 2019

Member

we went out of our way to leave this required when considering AutoDirectionProvider - the likelihood of a bug with someone forgetting to provide it is very high.

Anything wrapped in withDirection must get a direction, that's the sole purpose of the HOC.

direction: context[CHANNEL] ? context[CHANNEL].getState() : defaultDirection,
};
let direction = context[CHANNEL] ? context[CHANNEL].getState() : defaultDirection;
// console.log(props.direction);

This comment has been minimized.

Copy link
@ljharb

ljharb May 13, 2019

Member
Suggested change
// console.log(props.direction);
@@ -68,8 +71,7 @@ export default function withDirection(WrappedComponent) {
WithDirection.contextTypes = contextTypes;
WithDirection.displayName = `withDirection(${wrappedComponentName})`;
if (WrappedComponent.propTypes) {
WithDirection.propTypes = deepmerge({}, WrappedComponent.propTypes);
delete WithDirection.propTypes.direction;
WithDirection.propTypes = deepmerge(WrappedComponent.propTypes, withDirectionPropTypes);

This comment has been minimized.

Copy link
@ljharb

ljharb May 13, 2019

Member

it might be optional here, but it should stay required on the wrapped component.

@luis-mgb

This comment has been minimized.

Copy link
Author

commented May 13, 2019

The issue was solved up-stream and this is no longer needed

@luis-mgb luis-mgb closed this May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.