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

31695 theme switching composer #31831

Merged
merged 11 commits into from
Nov 29, 2023
9 changes: 6 additions & 3 deletions src/components/Composer/index.android.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import {StyleSheet} from 'react-native';
import _ from 'underscore';
import RNTextInput from '@components/RNTextInput';
import * as ComposerUtils from '@libs/ComposerUtils';
import themeColors from '@styles/themes/default';
import {useTheme} from '@styles/themes/useTheme';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing the following error after I synced with main

image

Shouldn't this be

import useTheme from '@styles/themes/useTheme';

There doesn't seem to be a named export called useTheme here: src/styles/themes/useTheme.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing the same thing. @lukemorawski why are there no testing steps and no screenshots on this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing the same thing. @lukemorawski why are there no testing steps and no screenshots on this PR?

Maybe because QA team cannot test this PR without switching to light theme which is not enabled yet.

But it would have been better to test in all platforms though this is very simple PR. Or have C+ review and test - should the bug have been caught earlier before merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, wrong auto import! fixed now

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a PR that fixes this already?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ah perfect, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this PR doesn't enable or change any functionality, but it does refactor theming in several components. We should always test these changes even if the test is a super simple "view each component and verify styling is still working". That would have been enough to catch small errors. QA regression testing should catch anything more complex.

@lukemorawski please don't cut corners and test on all platforms even for the simple stuff. I know it's annoying, but it's worth the time 🙇.

Copy link
Contributor

Choose a reason for hiding this comment

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

should have catched this in the review as well, sorry for that! 🫠

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally agree, sorry again for that

import useThemeStyles from '@styles/useThemeStyles';

const propTypes = {
/** Maximum number of lines in the text input */
Expand Down Expand Up @@ -65,6 +66,8 @@ const defaultProps = {

function Composer({shouldClear, onClear, isDisabled, maxLines, forwardedRef, isComposerFullSize, setIsFullComposerAvailable, ...props}) {
const textInput = useRef(null);
const theme = useTheme();
const styles = useThemeStyles();

/**
* Set the TextInput Ref
Expand Down Expand Up @@ -110,9 +113,9 @@ function Composer({shouldClear, onClear, isDisabled, maxLines, forwardedRef, isC
return (
<RNTextInput
autoComplete="off"
placeholderTextColor={themeColors.placeholderText}
placeholderTextColor={theme.placeholderText}
ref={setTextInputRef}
onContentSizeChange={(e) => ComposerUtils.updateNumberOfLines({maxLines, isComposerFullSize, isDisabled, setIsFullComposerAvailable}, e)}
onContentSizeChange={(e) => ComposerUtils.updateNumberOfLines({maxLines, isComposerFullSize, isDisabled, setIsFullComposerAvailable}, e, styles)}
rejectResponderTermination={false}
// Setting a really high number here fixes an issue with the `maxNumberOfLines` prop on TextInput, where on Android the text input would collapse to only one line,
// when it should actually expand to the container (https://github.com/Expensify/App/issues/11694#issuecomment-1560520670)
Expand Down
10 changes: 6 additions & 4 deletions src/components/Composer/index.ios.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import {StyleSheet} from 'react-native';
import _ from 'underscore';
import RNTextInput from '@components/RNTextInput';
import * as ComposerUtils from '@libs/ComposerUtils';
import styles from '@styles/styles';
import themeColors from '@styles/themes/default';
import useTheme from '@styles/themes/useTheme';
import useThemeStyles from '@styles/useThemeStyles';

const propTypes = {
/** If the input should clear, it actually gets intercepted instead of .clear() */
Expand Down Expand Up @@ -66,6 +66,8 @@ const defaultProps = {

function Composer({shouldClear, onClear, isDisabled, maxLines, forwardedRef, isComposerFullSize, setIsFullComposerAvailable, ...props}) {
const textInput = useRef(null);
const styles = useThemeStyles();
const theme = useTheme();

/**
* Set the TextInput Ref
Expand Down Expand Up @@ -115,9 +117,9 @@ function Composer({shouldClear, onClear, isDisabled, maxLines, forwardedRef, isC
return (
<RNTextInput
autoComplete="off"
placeholderTextColor={themeColors.placeholderText}
placeholderTextColor={theme.placeholderText}
ref={setTextInputRef}
onContentSizeChange={(e) => ComposerUtils.updateNumberOfLines({maxLines, isComposerFullSize, isDisabled, setIsFullComposerAvailable}, e)}
onContentSizeChange={(e) => ComposerUtils.updateNumberOfLines({maxLines, isComposerFullSize, isDisabled, setIsFullComposerAvailable}, e, styles)}
rejectResponderTermination={false}
smartInsertDelete={false}
maxNumberOfLines={maxNumberOfLines}
Expand Down
6 changes: 4 additions & 2 deletions src/components/EmojiPicker/EmojiPickerMenu/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ import compose from '@libs/compose';
import * as EmojiUtils from '@libs/EmojiUtils';
import getOperatingSystem from '@libs/getOperatingSystem';
import isEnterWhileComposition from '@libs/KeyboardShortcut/isEnterWhileComposition';
import styles from '@styles/styles';
import * as StyleUtils from '@styles/StyleUtils';
import useThemeStyles from '@styles/useThemeStyles';
import * as User from '@userActions/User';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
Expand Down Expand Up @@ -51,6 +51,8 @@ const throttleTime = Browser.isMobile() ? 200 : 50;
function EmojiPickerMenu(props) {
const {forwardedRef, frequentlyUsedEmojis, preferredSkinTone, onEmojiSelected, preferredLocale, translate} = props;

const styles = useThemeStyles();

const {isSmallScreenWidth, windowHeight} = useWindowDimensions();

// Ref for the emoji search input
Expand Down Expand Up @@ -462,7 +464,7 @@ function EmojiPickerMenu(props) {
/>
);
},
[isUsingKeyboardMovement, highlightedIndex, onEmojiSelected, preferredSkinTone, translate, highlightFirstEmoji],
[preferredSkinTone, highlightedIndex, isUsingKeyboardMovement, highlightFirstEmoji, styles, translate, onEmojiSelected],
);

const isFiltered = emojis.current.length !== filteredEmojis.length;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import getNumberOfLines from '@libs/ComposerUtils/getNumberOfLines';
import updateIsFullComposerAvailable from '@libs/ComposerUtils/updateIsFullComposerAvailable';
import styles from '@styles/styles';
import UpdateNumberOfLines from './types';

/**
* Check the current scrollHeight of the textarea (minus any padding) and
* divide by line height to get the total number of rows for the textarea.
*/
const updateNumberOfLines: UpdateNumberOfLines = (props, event) => {
const updateNumberOfLines: UpdateNumberOfLines = (props, event, styles) => {
const lineHeight = styles.textInputCompose.lineHeight ?? 0;
const paddingTopAndBottom = styles.textInputComposeSpacing.paddingVertical * 2;
const inputHeight = event?.nativeEvent?.contentSize?.height ?? null;
Expand Down
3 changes: 2 additions & 1 deletion src/libs/ComposerUtils/updateNumberOfLines/types.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {NativeSyntheticEvent, TextInputContentSizeChangeEventData} from 'react-native';
import ComposerProps from '@libs/ComposerUtils/types';
import themeStyles from '@styles/styles';

type UpdateNumberOfLines = (props: ComposerProps, event: NativeSyntheticEvent<TextInputContentSizeChangeEventData>) => void;
type UpdateNumberOfLines = (props: ComposerProps, event: NativeSyntheticEvent<TextInputContentSizeChangeEventData>, styles: typeof themeStyles) => void;

export default UpdateNumberOfLines;
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ function ComposerWithSuggestions({

return (
<>
<View style={[containerComposeStyles, styles.textInputComposeBorder]}>
<View style={[containerComposeStyles(styles), styles.textInputComposeBorder]}>
<Composer
checkComposerVisibility={checkComposerVisibility}
autoFocus={shouldAutoFocus}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ReportActionItemMessageEdit.js
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ function ReportActionItemMessageEdit(props) {
</PressableWithFeedback>
</Tooltip>
</View>
<View style={[containerComposeStyles, styles.textInputComposeBorder]}>
<View style={[containerComposeStyles(styles), styles.textInputComposeBorder]}>
<Composer
multiline
ref={(el) => {
Expand Down
3 changes: 1 addition & 2 deletions src/styles/containerComposeStyles/index.native.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import styles from '@styles/styles';
import ContainerComposeStyles from './types';

const containerComposeStyles: ContainerComposeStyles = [styles.textInputComposeSpacing];
const containerComposeStyles: ContainerComposeStyles = (styles) => [styles.textInputComposeSpacing];

export default containerComposeStyles;
3 changes: 1 addition & 2 deletions src/styles/containerComposeStyles/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import styles from '@styles/styles';
import ContainerComposeStyles from './types';

// We need to set paddingVertical = 0 on web to avoid displaying a normal pointer on some parts of compose box when not in focus
const containerComposeStyles: ContainerComposeStyles = [styles.textInputComposeSpacing, {paddingVertical: 0}];
const containerComposeStyles: ContainerComposeStyles = (styles) => [styles.textInputComposeSpacing, {paddingVertical: 0}];

export default containerComposeStyles;
3 changes: 2 additions & 1 deletion src/styles/containerComposeStyles/types.ts
lukemorawski marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {ViewStyle} from 'react-native';
import themeStyles from '@styles/styles';

type ContainerComposeStyles = ViewStyle[];
type ContainerComposeStyles = (styles: typeof themeStyles) => ViewStyle[];

export default ContainerComposeStyles;
Loading