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

feat(onboarding): accept children as props / retool the guide component #1

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

setsun
Copy link

@setsun setsun commented Jul 31, 2018

GROW 290

This PR repurposes Reactour for the onboarding flow for the player page.

Essentially what I have done is change the library to accept React children as props so it is easier to style the inner content.

Also I've added a tooltip nub / beacons which are styles specific to our onboarding. Eventually as we move towards onboarding on the dashboard page, I would want to consider moving the positioning logic inside of @frameio/components and make it easier to test it.

@setsun setsun requested a review from a team July 31, 2018 11:25
@@ -0,0 +1,25 @@
import React from 'react'

Choose a reason for hiding this comment

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

can we not get away with using CSS triangles? if we have to use SVGs, should that that svg in a separate file and fix webpack if need be

Choose a reason for hiding this comment

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

or rather, maybe change the tooltip to take a render prop so that we can let the consumer do whatever they want to the tooltip container?

Copy link
Author

Choose a reason for hiding this comment

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

I think this component on itself can remain the same, and I may change the <Guide /> component in the future to accept a pointer render prop

}
}

const hX = hx.isOutsideX(targetLeft + helperWidth, windowWidth)

Choose a reason for hiding this comment

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

i know you didn't write this, but these fiddly logic makes me a bit nervous without tests backing it

Copy link
Author

Choose a reason for hiding this comment

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

yeah...I think in the future I'll revisit this and see about making this more local to web-client-components.

@setsun setsun force-pushed the sl-onboarding branch 2 times, most recently from 9fbfc47 to 41bf612 Compare August 1, 2018 16:40
@setsun setsun merged commit 24ff808 into master Aug 1, 2018
@setsun setsun deleted the sl-onboarding branch August 1, 2018 17:05
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