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.

Sign up for GitHub

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

Fixes color picker segmented control rendering and scrolling issues #30994

Merged
merged 7 commits into from
May 7, 2021
Merged

Fixes color picker segmented control rendering and scrolling issues #30994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4ddd1d9
Fixes collor-palette scrolling issue
Apr 20, 2021
8613bc7
Uses existing ANIMATION_DURATION for clarity
Apr 26, 2021
047b279
Adds release note
Apr 27, 2021
974dd72
Revert "Uses existing ANIMATION_DURATION for clarity"
May 6, 2021
0aad64a
Revert "Fixes collor-palette scrolling issue"
May 6, 2021
27a314b
Revert "[RNMobile] SegmentedControls correction (#28740)"
May 6, 2021
9094f38
Adds delay before scrolling to counteract segmented control delay
May 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 51 additions & 60 deletions packages/components/src/color-palette/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { map, uniq } from 'lodash';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useRef, useEffect, useState } from '@wordpress/element';
import { useRef, useEffect } from '@wordpress/element';
import { usePreferredColorSchemeStyle } from '@wordpress/compose';
/**
* Internal dependencies
Expand Down Expand Up @@ -50,15 +50,6 @@ function ColorPalette( {
'linear-gradient(240deg, rgba(0,255,0,.8) 0%, rgba(0,255,0,0) 70.71%)',
'linear-gradient(360deg, rgba(0,0,255,.8) 0%, rgba(0,0,255,0) 70.71%)',
];
const isCustomGradientColor = isGradientColor && isSelectedCustom();

const [
shouldShowCustomIndicator,
setShouldShowCustomIndicator,
] = useState(
shouldShowCustomIndicatorOption &&
( ! isGradientSegment || isCustomGradientColor )
);

const scrollViewRef = useRef();
const isIOS = Platform.OS === 'ios';
Expand All @@ -77,17 +68,10 @@ function ColorPalette( {
const customIndicatorColor = isGradientSegment
? activeColor
: customSwatchGradients;

useEffect( () => {
setShouldShowCustomIndicator(
shouldShowCustomIndicatorOption &&
( ! isGradientSegment || isCustomGradientColor )
);
}, [
shouldShowCustomIndicatorOption,
isGradientSegment,
isCustomGradientColor,
] );
Comment on lines -86 to -90
Copy link
Member

Choose a reason for hiding this comment

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

I think reverting #28740 is likely a fine approach, but I am curious as to why it was not working.

I haven't explored it fully, but I wonder if the dependency array is not inclusive enough. It included isCustomGradientColor, but that value is computed with isSelectedCustom, which relies upon the values activeColor, colors, isGradientSegment, and isGradientColor.

I would think React would appropriately "watch" for changes to all those sub-dependencies of isSelectedCustom, but I wonder if maybe that is not occurring. Maybe we need to declare all of those values as dependencies for this hook?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for your feedback and reviewing this so quickly @dcalhoun 🙇
I was able to reproduce both the reported issues.

Maybe we need to declare all of those values as dependencies for this hook?

I will revisit this with this approach and report back 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @dcalhoun 👋
Thank you once more for the tip on this. Passing all the values and doing the calculation within the hook (85ff940) only fixes the disappearing custom gradient and not the original issue. Tbh I'm still a bit confused on why this happens only only compiled builds. I'll continue the investigation and get back 😕

Copy link
Member

@dcalhoun dcalhoun Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting. #30878 seems like either a style issue (i.e. some flex styles are conflicting or causing rendering issues), a calculation issue (i.e. there is somewhere else where we do not always re-calculate when we should), or a race condition (e.g. a computation occurs during layout and finishes at different times).

I might start by placing "debug styles" on each element to see which element is the offending item "cutting off" the children. Specifically, I know ScrollView contains multiple elements, a container and a content container. Which one is rendering too skinny?

Relatedly, the broken scroll-to-end for custom colors/gradients (wordpress-mobile/gutenberg-mobile#3104) is likely caused by the same issue where we compute the status within the hook, but do not have a dependency declared on the sub-dependency values.

gutenberg/packages/components/src/color-palette/index.native.js

Lines 102 to 110 in 85ff940

useEffect( () => {
if ( scrollViewRef.current ) {
if ( isSelectedCustom() ) {
scrollViewRef.current.scrollToEnd();
} else {
scrollViewRef.current.scrollTo( { x: 0, y: 0 } );
}
}
}, [ currentSegment ] );

Applying a similar change as 85ff940 would likely resolve the scroll issue.

const isCustomGradientColor = isGradientColor && isSelectedCustom();
const shouldShowCustomIndicator =
shouldShowCustomIndicatorOption &&
( ! isGradientSegment || isCustomGradientColor );

const accessibilityHint = isGradientSegment
? __( 'Navigates to customize the gradient' )
Expand All @@ -97,7 +81,7 @@ function ColorPalette( {
useEffect( () => {
if ( scrollViewRef.current ) {
if ( isSelectedCustom() ) {
scrollViewRef.current.scrollToEnd();
scrollToEndWithDelay();
} else {
scrollViewRef.current.scrollTo( { x: 0, y: 0 } );
}
Expand Down Expand Up @@ -180,10 +164,19 @@ function ColorPalette( {
function onContentSizeChange( width ) {
contentWidth = width;
if ( isSelectedCustom() && scrollViewRef.current ) {
scrollViewRef.current.scrollToEnd( { animated: ! isIOS } );
scrollToEndWithDelay();
}
}

function scrollToEndWithDelay() {
const delayedScroll = setTimeout( () => {
scrollViewRef.current.scrollToEnd();
}, ANIMATION_DURATION );
return () => {
clearTimeout( delayedScroll );
};
}

function onCustomIndicatorLayout( { nativeEvent } ) {
const { width } = nativeEvent.layout;
if ( width !== customIndicatorWidth ) {
Expand Down Expand Up @@ -225,6 +218,41 @@ function ColorPalette( {
onScrollEndDrag={ () => shouldEnableBottomSheetScroll( true ) }
ref={ scrollViewRef }
>
{ shouldShowCustomIndicator && (
<View
style={ customIndicatorWrapperStyle }
onLayout={ onCustomIndicatorLayout }
>
{ shouldShowCustomVerticalSeparator && (
<View style={ verticalSeparatorStyle } />
) }
<TouchableWithoutFeedback
onPress={ onCustomPress }
accessibilityRole={ 'button' }
accessibilityState={ { selected: isSelectedCustom() } }
accessibilityHint={ accessibilityHint }
>
<View style={ customIndicatorWrapperStyle }>
<ColorIndicator
withCustomPicker={ ! isGradientSegment }
color={ customIndicatorColor }
isSelected={ isSelectedCustom() }
style={ [
styles.colorIndicator,
customColorIndicatorStyles,
] }
/>
{ shouldShowCustomLabel && (
<Text style={ customTextStyle }>
{ isIOS
? customText
: customText.toUpperCase() }
</Text>
) }
</View>
</TouchableWithoutFeedback>
</View>
) }
{ colors.map( ( color ) => {
const scaleValue = isSelected( color ) ? scaleInterpolation : 1;
return (
Expand Down Expand Up @@ -260,43 +288,6 @@ function ColorPalette( {
</View>
);
} ) }
{ shouldShowCustomIndicator && (
<View
style={ customIndicatorWrapperStyle }
onLayout={ onCustomIndicatorLayout }
>
{ shouldShowCustomVerticalSeparator && (
<View style={ verticalSeparatorStyle } />
) }
<TouchableWithoutFeedback
onPress={ onCustomPress }
accessibilityRole={ 'button' }
accessibilityState={ {
selected: isSelectedCustom(),
} }
accessibilityHint={ accessibilityHint }
>
<View style={ customIndicatorWrapperStyle }>
<ColorIndicator
withCustomPicker={ ! isGradientSegment }
color={ customIndicatorColor }
isSelected={ isSelectedCustom() }
style={ [
styles.colorIndicator,
customColorIndicatorStyles,
] }
/>
{ shouldShowCustomLabel && (
<Text style={ customTextStyle }>
{ isIOS
? customText
: customText.toUpperCase() }
</Text>
) }
</View>
</TouchableWithoutFeedback>
</View>
) }
</ScrollView>
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/color-palette/style.native.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
.contentContainer {
flex-direction: row;
flex-direction: row-reverse;
padding: 0 $grid-unit-20;
}

Expand Down
1 change: 1 addition & 0 deletions packages/react-native-editor/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ For each user feature we should also add a importance categorization label to i

## Unreleased
- [*] Bottom-sheet: Add custom header [#30291]
- [*] Fixes color picker rendering bug when scrolling [#30994]

## 1.52.0

Expand Down