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

Refactor: Remove Avatar platform specific code #15663

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 7 additions & 7 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
"test:e2e": "node tests/e2e/testRunner.js --development"
},
"dependencies": {
"@expensify/react-native-web": "0.18.12",
"@expensify/react-native-web": "0.18.15",
"@formatjs/intl-getcanonicallocales": "^1.5.8",
"@formatjs/intl-locale": "^2.4.21",
"@formatjs/intl-numberformat": "^6.2.5",
Expand Down
8 changes: 1 addition & 7 deletions src/components/Avatar.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import themeColors from '../styles/themes/default';
import CONST from '../CONST';
import * as StyleUtils from '../styles/StyleUtils';
import * as Expensicons from './Icon/Expensicons';
import getAvatarDefaultSource from '../libs/getAvatarDefaultSource';
import Image from './Image';
import {withNetwork} from './OnyxProvider';
import networkPropTypes from './networkPropTypes';
Expand Down Expand Up @@ -119,12 +118,7 @@ class Avatar extends PureComponent {
</View>
)
: (
<Image
source={{uri: this.props.source}}
defaultSource={getAvatarDefaultSource(this.props.source)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The defaultSource workaround is no longer needed.
We just pass source

style={imageStyle}
onError={() => this.setState({imageError: true})}
/>
<Image source={{uri: this.props.source}} style={imageStyle} onError={() => this.setState({imageError: true})} />
)}
</View>
);
Expand Down
44 changes: 20 additions & 24 deletions src/components/Image/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,36 +8,23 @@ import {defaultProps, imagePropTypes} from './imagePropTypes';
import RESIZE_MODES from './resizeModes';

class Image extends React.Component {
constructor(props) {
super(props);

this.debouncedConfigureImageSource = _.debounce(this.configureImageSource, 220);

this.state = {
imageSource: undefined,
};
Comment on lines -16 to -18
Copy link
Contributor Author

@kidroca kidroca Mar 4, 2023

Choose a reason for hiding this comment

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

Having imageSource with undefined value initially would render no image for the first render

We don't have to use state for imageSource - it doesn't save us from re-renders, because most usages pass source like <Image soruce={{ uri: '...' }} /> (a new object created inline)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The debounce here was removed because it no longer has any effect (and was a workaround) as discussed here: #9279 (comment)

(Also it's inconsistent with the .native component where no debounce is used)

There are further changes that we can add to the Image component and I can open a follow up PR:

  1. remove configureOnLoad - this was a workaround needed because at the time react-native-web did not provide dimension in the original onLoad callback
  2. Turn Image into a functional component - no lifecycle and state are used.

I do both of this changes here:

but the PR is on hold

}

componentDidMount() {
this.debouncedConfigureImageSource();
this.configureOnLoad();
}

componentDidUpdate(prevProps) {
if (prevProps.source.uri === this.props.source.uri) {
if (prevProps.source === this.props.source) {
return;
}

this.debouncedConfigureImageSource.cancel();
this.debouncedConfigureImageSource();
this.configureOnLoad();
}

/**
* Check if the image source is a URL - if so the `encryptedAuthToken` is appended
* to the source. The natural image dimensions can then be retrieved using this source
* and as a result the `onLoad` event needs to be maunually invoked to return these dimensions
* to the source.
* @returns {Object} - the configured image source
*/
configureImageSource() {
this.props.onLoadStart();
getImageSource() {
const source = this.props.source;
let imageSource = source;
if (this.props.isAuthTokenRequired) {
Expand All @@ -47,25 +34,34 @@ class Image extends React.Component {
const authToken = lodashGet(this.props, 'session.encryptedAuthToken', null);
imageSource = {uri: `${source.uri}?encryptedAuthToken=${encodeURIComponent(authToken)}`};
}
this.setState({imageSource});

return imageSource;
}

/**
* The natural image dimensions are retrieved using the updated source
* and as a result the `onLoad` event needs to be manually invoked to return these dimensions
*/
configureOnLoad() {
// If an onLoad callback was specified then manually call it and pass
// the natural image dimensions to match the native API
if (this.props.onLoad == null) {
return;
}

const imageSource = this.getImageSource();
RNImage.getSize(imageSource.uri, (width, height) => {
this.props.onLoad({nativeEvent: {width, height}});
});
}

render() {
// eslint-disable-next-line
const { source, onLoad, ...rest } = this.props;
// Omit the props which the underlying RNImage won't use
const forwardedProps = _.omit(this.props, ['source', 'onLoad', 'session', 'isAuthTokenRequired']);
const source = this.getImageSource();

// eslint-disable-next-line
return <RNImage {...rest} source={this.state.imageSource} />;
// eslint-disable-next-line react/jsx-props-no-spreading
return <RNImage {...forwardedProps} source={source} />;
}
}

Expand Down
7 changes: 0 additions & 7 deletions src/libs/getAvatarDefaultSource/index.js

This file was deleted.

5 changes: 0 additions & 5 deletions src/libs/getAvatarDefaultSource/index.native.js

This file was deleted.