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

Jump to bottom

Feat/31678 theme switching migration selectionlist batch #31774

Merged
merged 19 commits into from
Nov 29, 2023
Merged

Feat/31678 theme switching migration selectionlist batch #31774

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
499a416
feat: migrate MultipleAvatars
koko57 Nov 22, 2023
dbe88eb
fix: migrate SelectCircle
koko57 Nov 22, 2023
d38b9e3
feat: migrate OfflineWithFeedback
koko57 Nov 22, 2023
c3fa464
feat: migrate BaseOptionsSelector
koko57 Nov 22, 2023
c8a9777
feat: migrate optionRowStyles
koko57 Nov 22, 2023
e610716
fix: minor fix for MultipleAvatars, prettier
koko57 Nov 22, 2023
9c09583
feat: migrate Icon
koko57 Nov 23, 2023
b0e476c
Merge branch 'main' into feat/31678-theme-switching-migration-selecti…
koko57 Nov 23, 2023
a5ab9ef
fix: lint
koko57 Nov 24, 2023
ccf1c26
fix: add missing dep
koko57 Nov 24, 2023
2ff63d1
fix: apply requested changes
koko57 Nov 24, 2023
422fb8e
fix: apply requested changes
koko57 Nov 24, 2023
b6ab866
fix: lint
koko57 Nov 24, 2023
b6c2a4e
feat: migrate BaseOnfidoWeb
koko57 Nov 27, 2023
744c881
fix: revert last commit
koko57 Nov 27, 2023
49c4940
fix: resolve conflicts
koko57 Nov 28, 2023
3a3879c
fix: add missing proptypes
koko57 Nov 29, 2023
ad2b3b5
fix: rename a const, add an alias for a style prop
koko57 Nov 29, 2023
bd315c8
fix: run prettier
koko57 Nov 29, 2023
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
10 changes: 5 additions & 5 deletions src/components/Icon/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, {PureComponent} from 'react';
import {StyleProp, View, ViewStyle} from 'react-native';
import styles from '@styles/styles';
import withThemeStyles, {ThemeStylesProps} from '@components/withThemeStyles';
import * as StyleUtils from '@styles/StyleUtils';
import themeColors from '@styles/themes/default';
import variables from '@styles/variables';
Expand Down Expand Up @@ -41,7 +41,7 @@ type IconProps = {

/** Additional styles to add to the Icon */
additionalStyles?: StyleProp<ViewStyle>;
};
} & ThemeStylesProps;

// We must use a class component to create an animatable component with the Animated API
// eslint-disable-next-line react/prefer-stateless-function
Expand All @@ -61,13 +61,13 @@ class Icon extends PureComponent<IconProps> {
render() {
const width = this.props.small ? variables.iconSizeSmall : this.props.width;
const height = this.props.small ? variables.iconSizeSmall : this.props.height;
const iconStyles = [StyleUtils.getWidthAndHeightStyle(width ?? 0, height), IconWrapperStyles, styles.pAbsolute, this.props.additionalStyles];
const iconStyles = [StyleUtils.getWidthAndHeightStyle(width ?? 0, height), IconWrapperStyles, this.props.themeStyles.pAbsolute, this.props.additionalStyles];

if (this.props.inline) {
return (
<View
testID={`${this.props.src.name} Icon`}
style={[StyleUtils.getWidthAndHeightStyle(width ?? 0, height), styles.bgTransparent, styles.overflowVisible]}
style={[StyleUtils.getWidthAndHeightStyle(width ?? 0, height), this.props.themeStyles.bgTransparent, this.props.themeStyles.overflowVisible]}
>
<View style={iconStyles}>
<this.props.src
Expand Down Expand Up @@ -99,4 +99,4 @@ class Icon extends PureComponent<IconProps> {
}
}

export default Icon;
export default withThemeStyles(Icon);
2 changes: 1 addition & 1 deletion src/components/LHNOptionsList/OptionRowLHN.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ function OptionRowLHN(props) {
props.style,
);
const contentContainerStyles =
props.viewMode === CONST.OPTION_MODE.COMPACT ? [styles.flex1, styles.flexRow, styles.overflowHidden, optionRowStyles.compactContentContainerStyles] : [styles.flex1];
props.viewMode === CONST.OPTION_MODE.COMPACT ? [styles.flex1, styles.flexRow, styles.overflowHidden, optionRowStyles.compactContentContainerStyles(styles)] : [styles.flex1];
const sidebarInnerRowStyle = StyleSheet.flatten(
props.viewMode === CONST.OPTION_MODE.COMPACT
? [styles.chatLinkRowPressable, styles.flexGrow1, styles.optionItemAvatarNameWrapper, styles.optionRowCompact, styles.justifyContentCenter]
Expand Down
60 changes: 35 additions & 25 deletions src/components/MultipleAvatars.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import React, {memo, useMemo} from 'react';
import {StyleProp, View, ViewStyle} from 'react-native';
import {ValueOf} from 'type-fest';
import {AvatarSource} from '@libs/UserUtils';
import styles from '@styles/styles';
import * as StyleUtils from '@styles/StyleUtils';
import themeColors from '@styles/themes/default';
import useTheme from '@styles/themes/useTheme';
import useThemeStyles from '@styles/useThemeStyles';
import variables from '@styles/variables';
import CONST from '@src/CONST';
import type {Icon} from '@src/types/onyx/OnyxCommon';
Expand Down Expand Up @@ -63,26 +63,11 @@ type AvatarSizeToStyles = typeof CONST.AVATAR_SIZE.SMALL | typeof CONST.AVATAR_S

type AvatarSizeToStylesMap = Record<AvatarSizeToStyles, AvatarStyles>;

const avatarSizeToStylesMap: AvatarSizeToStylesMap = {
[CONST.AVATAR_SIZE.SMALL]: {
singleAvatarStyle: styles.singleAvatarSmall,
secondAvatarStyles: styles.secondAvatarSmall,
},
[CONST.AVATAR_SIZE.LARGE]: {
singleAvatarStyle: styles.singleAvatarMedium,
secondAvatarStyles: styles.secondAvatarMedium,
},
[CONST.AVATAR_SIZE.DEFAULT]: {
singleAvatarStyle: styles.singleAvatar,
secondAvatarStyles: styles.secondAvatar,
},
};

function MultipleAvatars({
fallbackIcon,
icons = [],
size = CONST.AVATAR_SIZE.DEFAULT,
secondAvatarStyle = [StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)],
secondAvatarStyle,
shouldStackHorizontally = false,
shouldDisplayAvatarsInRows = false,
isHovered = false,
Expand All @@ -93,8 +78,31 @@ function MultipleAvatars({
shouldUseCardBackground = false,
maxAvatarsInRow = CONST.AVATAR_ROW_SIZE.DEFAULT,
}: MultipleAvatarsProps) {
const theme = useTheme();
const styles = useThemeStyles();

const avatarSizeToStylesMap: AvatarSizeToStylesMap = useMemo(
() => ({
[CONST.AVATAR_SIZE.SMALL]: {
singleAvatarStyle: styles.singleAvatarSmall,
secondAvatarStyles: styles.secondAvatarSmall,
},
[CONST.AVATAR_SIZE.LARGE]: {
singleAvatarStyle: styles.singleAvatarMedium,
secondAvatarStyles: styles.secondAvatarMedium,
},
[CONST.AVATAR_SIZE.DEFAULT]: {
singleAvatarStyle: styles.singleAvatar,
secondAvatarStyles: styles.secondAvatar,
},
}),
[styles],
);

const secondAvatarDefaultStyles = secondAvatarStyle ?? [StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rename this variable to secondAvatarStyle and rename the prop secondAvatarStyle to secondAvatarStyleProp.

Especially i think this should be singular instead of plural, since the prop is also singular. (sorry for nitpicking 😃)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it would break the convention we have in the app - all the styles passed as props doesn't have 'props' in the name. badgeStyles, textStyles, containerStyles or even listContainerStyles in BaseOptionsSelector below - these are just some of examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

which convention do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Naming the props. I just gave some examples how the style props are named. We don't have containerStylesProp only containerStyles. If I changed secondAvatarStyle to secondAvatarStyleProp that would be the only style prop in the app which name includes 'Prop'

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to add Prop to the variable name, but if secondAvatarStyle can be an array of styles, maybe we rename to secondAvatarStyles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be safer to work on the names refactor in a separate PR 🙂

Copy link
Contributor

@chrispader chrispader Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

oh sorry, i might not have explained this well... @koko57 @grgia

i meant to create an alias for the prop just in this component. Like this:

function MultipleAvatars({
     ...
     secondAvatarStyle: secondAvatarStyleProp,
     ...
}) {
...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I agree, no need for name refactor here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, ok then 😃 thanks for clarification

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrispader changes applied 🙂


let avatarContainerStyles = StyleUtils.getContainerStyles(size, isInReportAction);
const {singleAvatarStyle, secondAvatarStyles} = useMemo(() => avatarSizeToStylesMap[size as AvatarSizeToStyles] ?? avatarSizeToStylesMap.default, [size]);
const {singleAvatarStyle, secondAvatarStyles} = useMemo(() => avatarSizeToStylesMap[size as AvatarSizeToStyles] ?? avatarSizeToStylesMap.default, [size, avatarSizeToStylesMap]);

const tooltipTexts = useMemo(() => (shouldShowTooltip ? icons.map((icon) => icon.name) : ['']), [shouldShowTooltip, icons]);
const avatarSize = useMemo(() => {
Expand Down Expand Up @@ -143,7 +151,7 @@ function MultipleAvatars({
<Avatar
source={icons[0].source}
size={size}
fill={themeColors.iconSuccessFill}
fill={theme.iconSuccessFill}
name={icons[0].name}
type={icons[0].type}
fallbackIcon={icons[0].fallbackIcon}
Expand Down Expand Up @@ -193,7 +201,7 @@ function MultipleAvatars({
StyleUtils.getAvatarBorderWidth(size),
]}
source={icon.source ?? fallbackIcon}
fill={themeColors.iconSuccessFill}
fill={theme.iconSuccessFill}
size={size}
name={icon.name}
type={icon.type}
Expand All @@ -219,7 +227,7 @@ function MultipleAvatars({
}),

// Set overlay background color with RGBA value so that the text will not inherit opacity
StyleUtils.getBackgroundColorWithOpacityStyle(themeColors.overlay, variables.overlayOpacity),
StyleUtils.getBackgroundColorWithOpacityStyle(theme.overlay, variables.overlayOpacity),
StyleUtils.getHorizontalStackedOverlayAvatarStyle(oneAvatarSize, oneAvatarBorderWidth),
icons[3].type === CONST.ICON_TYPE_WORKSPACE ? StyleUtils.getAvatarBorderRadius(size, icons[3].type) : {},
]}
Expand Down Expand Up @@ -256,7 +264,7 @@ function MultipleAvatars({
<View>
<Avatar
source={icons[0].source ?? fallbackIcon}
fill={themeColors.iconSuccessFill}
fill={theme.iconSuccessFill}
size={avatarSize}
imageStyles={[singleAvatarStyle]}
name={icons[0].name}
Expand All @@ -265,7 +273,9 @@ function MultipleAvatars({
/>
</View>
</UserDetailsTooltip>
<View style={[secondAvatarStyles, secondAvatarStyle, icons[1].type === CONST.ICON_TYPE_WORKSPACE ? StyleUtils.getAvatarBorderRadius(size, icons[1].type) : {}]}>
<View
style={[secondAvatarStyles, secondAvatarDefaultStyles, icons[1].type === CONST.ICON_TYPE_WORKSPACE ? StyleUtils.getAvatarBorderRadius(size, icons[1].type) : {}]}
>
{icons.length === 2 ? (
<UserDetailsTooltip
accountID={icons[1].id}
Expand All @@ -277,7 +287,7 @@ function MultipleAvatars({
<View>
<Avatar
source={icons[1].source ?? fallbackIcon}
fill={themeColors.iconSuccessFill}
fill={theme.iconSuccessFill}
size={avatarSize}
imageStyles={[singleAvatarStyle]}
name={icons[1].name}
Expand Down
12 changes: 7 additions & 5 deletions src/components/OfflineWithFeedback.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import PropTypes from 'prop-types';
import React from 'react';
import React, {useCallback} from 'react';

Check failure on line 2 in src/components/OfflineWithFeedback.js

View workflow job for this annotation

GitHub Actions / lint 

'useCallback' is defined but never used
import {View} from 'react-native';
import _ from 'underscore';
import useNetwork from '@hooks/useNetwork';
import shouldRenderOffscreen from '@libs/shouldRenderOffscreen';
import stylePropTypes from '@styles/stylePropTypes';
import styles from '@styles/styles';
import * as StyleUtils from '@styles/StyleUtils';
import useThemeStyles from '@styles/useThemeStyles';
import CONST from '@src/CONST';
import MessagesRow from './MessagesRow';

Expand Down Expand Up @@ -76,22 +76,24 @@
/**
* This method applies the strikethrough to all the children passed recursively
* @param {Array} children
* @param {Object} styles
* @return {Array}
*/
function applyStrikeThrough(children) {
function applyStrikeThrough(children, styles) {
return React.Children.map(children, (child) => {
if (!React.isValidElement(child)) {
return child;
}
const props = {style: StyleUtils.combineStyles(child.props.style, styles.offlineFeedback.deleted, styles.userSelectNone)};
if (child.props.children) {
props.children = applyStrikeThrough(child.props.children);
props.children = applyStrikeThrough(child.props.children, styles);
}
return React.cloneElement(child, props);
});
}

function OfflineWithFeedback(props) {
const styles = useThemeStyles();
const {isOffline} = useNetwork();

const hasErrors = !_.isEmpty(props.errors);
Expand All @@ -109,7 +111,7 @@

// Apply strikethrough to children if needed, but skip it if we are not going to render them
if (needsStrikeThrough && !hideChildren) {
children = applyStrikeThrough(children);
children = applyStrikeThrough(children, styles);
}
return (
<View style={props.style}>
Expand Down
Loading
Loading