Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
dariusz-biela
left a comment
dariusz-biela
left a comment
There was a problem hiding this comment.
Overall, I think this PR brings very good changes. I’ve added a few comments that might be worth considering.
…rRadius into shouldRemoveBorderRadius
# Conflicts: # src/components/ButtonWithDropdownMenu/index.tsx
| variant === 'success' ? styles.buttonSuccess : undefined, | ||
| variant === 'danger' ? styles.buttonDanger : undefined, |
There was a problem hiding this comment.
const ButtonVariantStyles: Record<Variant, ...> = {
success: styles.buttonSuccess,
danger: styles. buttonDanger,
link: styles.bgTransparent,
default: undefined,
}In total, I would define the style configuration for variants
and ensure that the variant value in the component could not be undefined, but would have a default value of ‘default’.
There was a problem hiding this comment.
I think that this one could be a good addition 👍
There was a problem hiding this comment.
I moved it in a recent commit, please take a look 👀
| isDisabled && !shouldStayNormalOnDisable ? styles.buttonOpacityDisabled : undefined, | ||
| isDisabled && variant !== 'danger' && variant !== 'success' && !shouldStayNormalOnDisable ? styles.buttonDisabled : undefined, |
There was a problem hiding this comment.
for the disabled state :
const DisabledButtonVariantStyles: Record<Variant, ...> = {
success: [styles.buttonOpacityDisabled, styles.buttonSuccess],
danger: [styles.buttonOpacityDisabled, styles. buttonDanger],
link: [styles.buttonOpacityDisabled, styles.buttonDisabled, styles.bgTransparent],
default: [styles.buttonOpacityDisabled, styles.buttonDisabled],
}There was a problem hiding this comment.
So we would use it like isDisabled && !shouldStayNormalOnDisable ? DisabledButtonVariantStyles[variant] : ButtonVariantStyles[variant] ? I think it should work 👍
There was a problem hiding this comment.
I moved it in a recent commit, please take a look 👀
| <PressableWithFeedback | ||
| dataSet={{ | ||
| listener: pressOnEnter ? CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey : undefined, | ||
| }} | ||
| ref={ref as PressableRef} | ||
| onLayout={onLayout} | ||
| onPress={(event) => { | ||
| if (event?.type === 'click') { | ||
| const currentTarget = event?.currentTarget as HTMLElement; | ||
| currentTarget?.blur(); | ||
| } | ||
|
|
||
| if (shouldEnableHapticFeedback) { | ||
| HapticFeedback.press(); | ||
| } | ||
|
|
||
| if (isDisabled || isLoading) { | ||
| return; | ||
| } | ||
| return onPress(event); | ||
| }} | ||
| onLongPress={(event) => { | ||
| if (isLongPressDisabled) { | ||
| return; | ||
| } | ||
| if (shouldEnableHapticFeedback) { | ||
| HapticFeedback.longPress(); | ||
| } | ||
| onLongPress(event); | ||
| }} | ||
| onPressIn={onPressIn} | ||
| onPressOut={onPressOut} | ||
| onMouseDown={onMouseDown} | ||
| shouldBlendOpacity={shouldBlendOpacity} | ||
| disabled={isLoading || isDisabled} | ||
| wrapperStyle={[ | ||
| isDisabled && !shouldStayNormalOnDisable ? {...styles.cursorDisabled, ...styles.noSelect} : {}, | ||
| styles.buttonContainer, | ||
| shouldRemoveBorderRadius === 'right' || shouldRemoveBorderRadius === 'all' ? styles.noRightBorderRadius : undefined, | ||
| shouldRemoveBorderRadius === 'left' || shouldRemoveBorderRadius === 'all' ? styles.noLeftBorderRadius : undefined, | ||
| style, | ||
| ]} | ||
| style={buttonStyles} | ||
| isNested={isNested} | ||
| hoverStyle={ | ||
| !isDisabled || !shouldStayNormalOnDisable | ||
| ? [ | ||
| shouldUseDefaultHover && !isDisabled ? styles.buttonDefaultHovered : undefined, | ||
| variant === 'success' && !isDisabled ? styles.buttonSuccessHovered : undefined, | ||
| variant === 'danger' && !isDisabled ? styles.buttonDangerHovered : undefined, | ||
| hoverStyles, | ||
| ] | ||
| : [] | ||
| } | ||
| disabledStyle={!shouldStayNormalOnDisable ? disabledStyle : undefined} | ||
| id={id} | ||
| testID={testID} | ||
| accessibilityLabel={accessibilityLabel} | ||
| role={getButtonRole(isNested)} | ||
| hoverDimmingValue={1} | ||
| onHoverIn={!isDisabled || !shouldStayNormalOnDisable ? () => setIsHovered(true) : undefined} | ||
| onHoverOut={!isDisabled || !shouldStayNormalOnDisable ? () => setIsHovered(false) : undefined} | ||
| sentryLabel={sentryLabel} | ||
| > |
There was a problem hiding this comment.
I am considering sorting props into groups.
<PressableWithFeedback
dataSet={{
listener: pressOnEnter ? CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey : undefined,
}}
ref={ref as PressableRef}
id={id}
testID={testID}
sentryLabel={sentryLabel}
accessibilityLabel={accessibilityLabel}
role={getButtonRole(isNested)}
isNested={isNested}
disabled={isLoading || isDisabled}
shouldBlendOpacity={shouldBlendOpacity}
hoverDimmingValue={1}
wrapperStyle={[
isDisabled && !shouldStayNormalOnDisable ? {...styles.cursorDisabled, ...styles.noSelect} : {},
styles.buttonContainer,
shouldRemoveBorderRadius === 'right' || shouldRemoveBorderRadius === 'all' ? styles.noRightBorderRadius : undefined,
shouldRemoveBorderRadius === 'left' || shouldRemoveBorderRadius === 'all' ? styles.noLeftBorderRadius : undefined,
style,
]}
style={buttonStyles}
hoverStyle={
!isDisabled || !shouldStayNormalOnDisable
? [
shouldUseDefaultHover && !isDisabled ? styles.buttonDefaultHovered : undefined,
variant === 'success' && !isDisabled ? styles.buttonSuccessHovered : undefined,
variant === 'danger' && !isDisabled ? styles.buttonDangerHovered : undefined,
hoverStyles,
]
: []
}
disabledStyle={!shouldStayNormalOnDisable ? disabledStyle : undefined}
onLayout={onLayout}
onHoverIn={!isDisabled || !shouldStayNormalOnDisable ? () => setIsHovered(true) : undefined}
onHoverOut={!isDisabled || !shouldStayNormalOnDisable ? () => setIsHovered(false) : undefined}
onMouseDown={onMouseDown}
onPressIn={onPressIn}
onPressOut={onPressOut}
onPress={(event) => {
if (event?.type === 'click') {
const currentTarget = event?.currentTarget as HTMLElement;
currentTarget?.blur();
}
if (shouldEnableHapticFeedback) {
HapticFeedback.press();
}
if (isDisabled || isLoading) {
return;
}
return onPress(event);
}}
onLongPress={(event) => {
if (isLongPressDisabled) {
return;
}
if (shouldEnableHapticFeedback) {
HapticFeedback.longPress();
}
onLongPress(event);
}}
>There was a problem hiding this comment.
Yeah, I think it could be nice; I don't feel strongly about it, but I'll add it and see if it sticks 👍
There was a problem hiding this comment.
I'm not sure if prettier will let it stay, I'll reorder them though
| }; | ||
|
|
||
| function ButtonComposedIconLeft({src, fill, hoverFill, style}: ButtonComposedIconLeftProps) { | ||
| const {isHovered, variant, size} = useButtonComposedContext(); |
There was a problem hiding this comment.
move this hook to ButtonIconBase
There was a problem hiding this comment.
I changed it to get called also inside BaseIcon, but also left it in Left/Right Icon for marginStyle calculation
| allowBubble?: boolean; | ||
|
|
||
| /** Whether button's content should be centered */ | ||
| isContentCentered?: boolean; |
There was a problem hiding this comment.
can't this be entirely up to the consumer?
There was a problem hiding this comment.
Yes it can, actually even inside original Button component - I'll use iconWrapperStyles (contentContainerStyle in ComposedButton) to override justifyContent style 👍
| const isFocused = useIsFocused(); | ||
| const activeElementRole = useActiveElementRole(); | ||
|
|
||
| const shouldDisableEnterShortcut = useMemo(() => accessibilityRoles.includes(activeElementRole ?? '') && activeElementRole !== CONST.ROLE.PRESENTATION, [activeElementRole]); |
There was a problem hiding this comment.
why include any manual memoization?
There was a problem hiding this comment.
KeyboardShortcutComponent isn't memoized by React Compiler because of the react-hooks/exhaustive-deps rule linked to config deps - this logic was written more than 2 years ago, so if we removed it we would be prone to even more regressions. Both shouldDisableEnterShortcut and keyboardShortcutCallback are already manually memoized in original Button, so I wouldn't change it, at least not here.
| {children} | ||
| </View> | ||
| </ButtonComposedContext.Provider> | ||
| {isLoading && ( |
There was a problem hiding this comment.
this should be a separate component already if we plan to make this composable. loading state is up to the consumer
|
|
||
| return ( | ||
| <> | ||
| {pressOnEnter && ( |
There was a problem hiding this comment.
same here, this should live outside. first risk I see is making every single button on a page to render on isFocused when it doesn't have to
| innerStyles={[shouldUseNarrowLayout && styles.alignItemsCenter]} | ||
| style={shouldUseNarrowLayout ? [styles.flexGrow1, styles.mb3] : undefined} | ||
| /> | ||
| <ButtonWithDropdownMenu |
There was a problem hiding this comment.
we're not reworking this component's API (yet), right?
|
I just went through this PR and started replying to comments, I think the biggest pain point here will be styles, which are currently misplaced (at least compared to the original Button children's styles), I'll deal with the most important comments first and then start working over fixing the nested View's styles 👷♂️ |
…-fork into war-in/composable-button
…nu second button with secondText, fix style props
4 participants
Explanation of Change
Fixed Issues
$ #83762
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari