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

Ability to disable individual control items. #50

Merged
merged 6 commits into from
Oct 27, 2017
Merged

Ability to disable individual control items. #50

merged 6 commits into from
Oct 27, 2017

Conversation

fendorio
Copy link
Contributor

@fendorio fendorio commented Oct 1, 2017

Hey Kyle,

See below for the issue we were talking about last week.

Sorry for the delayed response, I've been completely swamped with worth this past week (still am!).

I agree that the logic to render a control or not should ideally be outside of the render function. I have just run some tests with the shorthand method you gave as an example, turns out that returning null does effect the flex-box layout, unlike the first approach.

See my test code in the branch '2configurableControls' inside my fork, please forgive the branch names ha ha!!

I've taken both of our approaches in a third branch inside my fork, called... you guessed it '3configurableControls', in this branch I perform the conditional logic for the controls outside of the JSX, storing the result of each conditional in a local variable, which the return statement's JSX references.

See an example snippet below for what i've done in this latest branch

/**
 * Renders an empty control, used to disable a control without breaking the view layout.
 */
renderNullControl() {
    return this.renderControl(<View></View>);   
}

/**
 * Groups the top bar controls together in an animated
 * view and spaces them out.
 */
renderTopControls() {

    const backControl = !this.props.disableBack ? this.renderBack() : this.renderNullControl();
    const volumeControl = !this.props.disableVolume ? this.renderVolume() : this.renderNullControl();
    const fullscreenControl = !this.props.disableFullscreen ? this.renderFullscreen() : this.renderNullControl();

    return(
        <Animated.View style={[
            styles.controls.top,
            {
                opacity: this.animations.topControl.opacity,
                marginTop: this.animations.topControl.marginTop,
            }
        ]}>
            <Image
                source={ require( './assets/img/top-vignette.png' ) }
                style={[ styles.controls.column, styles.controls.vignette,
            ]}>
                <View style={ styles.controls.topControlGroup }>
                    { backControl }
                    <View style={ styles.controls.pullRight }>
                        { volumeControl }
                        { fullscreenControl }
                    </View>
                </View>
            </Image>
        </Animated.View>
    );
}

This keeps the separation of concerns between the render top/bottom methods and the render *individual control methods, it also does not break the view layout as there is still technically a control in each ones place, just an empty one if a control has been disabled.

Hope that makes sense. Sorry again about the delay in response. Fire away any questions, happy to discuss further if needed.

Lastly, I like the approach you're talking about at the top of your response. I'd be happy to pitch in on making that happen in the future.

Added props documentation for hiding individual controls.
…de of the JSX statements in the render top/bottom methods. Added a renderNullControl method which renders an empty View element in-place of a control if it has been disabled, this is rendered in-place of a control if the respective disableX (e.g. disableVolume) prop has been passed to the VideoPlayer component
@fendorio
Copy link
Contributor Author

Had a chance to glance over this yet?

@kylemilloy
Copy link
Contributor

No sorry....I have been completely slammed the last couple of weeks...project is wrapping up today though so I'll review sometime this week.

@kylemilloy kylemilloy merged commit e4f4698 into itsnubix:master Oct 27, 2017
@kylemilloy
Copy link
Contributor

Looks good. Thanks again. Sorry for the delay.....

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