Skip to content

Hooks 🎉 #3

Merged
samuelcastro merged 3 commits intosamuelcastro:masterfrom
CarsonF:feature/hooks
Aug 22, 2019
Merged

Hooks 🎉 #3
samuelcastro merged 3 commits intosamuelcastro:masterfrom
CarsonF:feature/hooks

Conversation

@CarsonF
Copy link
Copy Markdown
Contributor

@CarsonF CarsonF commented Aug 9, 2019

useSplitTreatment

const [treatment, config] = useSplitTreatment('my-split', { optionalAttributes });
return treatment === 'on' ? <MyFeature /> : null;

I didn't see a need to include multiple splits because the hook can just be called multiple times, which is inline with what React suggests.
Maybe it would be good to return it as an array/tuple instead that way the individual treatments can be relabeled easier? Changed to that.

useSplitTrack

// Same track function as the client has
const track = useSplitTrack();
const handleClick = () => {
  track('button_clicked', ...);
};

Maybe useTrack would be better? And if it's ambiguous people can rename on import? I guess same goes for the other hook too.

@CarsonF CarsonF changed the title Hooks 🎉 Hooks 🎉 Aug 9, 2019
@CarsonF
Copy link
Copy Markdown
Contributor Author

CarsonF commented Aug 9, 2019

This kinda depends on #2, but I know that one has breaking changes. If we want this to go in first I should change the client check.

Copy link
Copy Markdown
Owner

@samuelcastro samuelcastro left a comment

Choose a reason for hiding this comment

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

It looks great, you mentioned breaking changes, do we have any breaking changes here?

@CarsonF
Copy link
Copy Markdown
Contributor Author

CarsonF commented Aug 16, 2019

None here 😄

@samuelcastro
Copy link
Copy Markdown
Owner

Another thing, we need to update the README to include the hooks usage, could you do that?

@CarsonF
Copy link
Copy Markdown
Contributor Author

CarsonF commented Aug 22, 2019

Added docs and renamed the hooks.

We might still have a discrepancy with what's returned by default from useSplit and Split, the "control" vs null thing.

Comment thread README.md Outdated
Copy link
Copy Markdown
Owner

@samuelcastro samuelcastro left a comment

Choose a reason for hiding this comment

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

Great!

@samuelcastro samuelcastro merged commit f48c23e into samuelcastro:master Aug 22, 2019
@CarsonF CarsonF deleted the feature/hooks branch August 22, 2019 21:40
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