Skip to content

Conversation

@YosefAshenafi
Copy link
Contributor

Added device info menu for the expo project

Copy link
Contributor

@shreyastelkar shreyastelkar left a comment

Choose a reason for hiding this comment

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

LFTM

jontsai
jontsai previously approved these changes Oct 16, 2024
{ title: 'Total Memory (Bytes)', identifier: 'totalMemory' },
{ title: 'Uptime', identifier: 'uptime' },
{ title: 'Jail Broken', identifier: 'isJailBroken' },
]; No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing empty line at the end of the file

TABLET = 2,
DESKTOP = 3,
TV = 4,
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing empty line at the end of the file

Comment on lines 32 to 36
Promise.all([
Device.getDeviceTypeAsync(),
Device.getUptimeAsync(),
Device.isRootedExperimentalAsync(),
])
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use separate promises. This will fail on all, even if only one promise fails.


import { Card, Text, View } from 'react-native-ui-lib';

import { useDeviceInfo } from 'htk/features/expo/deviceInfo/hooks';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use relative imports since this is a repo that is being used as submodule.
This is forcing us to use htk path alias. We still did not discuss about the standard path alias.

@@ -0,0 +1,66 @@
import { useEffect, useState } from 'react';

import { enumToStr } from 'htk/utils/enum';
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Let's use relative imports.

});

useEffect(() => {
const fetchDeviceInfo = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect. Please use .then() structure.

Device.getDeviceTypeAsync().then(deviceType => {
    setDeviceInfo(prev => ({...prev, deviceType: enumToStr(DeviceType, deviceType})
})).catch(error => {
    console.error('Failed to fetch device type:', error);
});

Please apply to the other async functions as well.

Copy link
Contributor

@gwainor gwainor left a comment

Choose a reason for hiding this comment

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

Also please fix you IDE to apply prettier on save or something.

Your prettier is not formatting the code.

Comment on lines 32 to 33
Device.getDeviceTypeAsync().then(deviceType => {
setDeviceInfo(prev => ({...prev, deviceType: deviceType ? enumToStr(DeviceType, deviceType) : null}));
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is a null possibility of the deviceType variable, there is no need to update the state.

Suggested change
Device.getDeviceTypeAsync().then(deviceType => {
setDeviceInfo(prev => ({...prev, deviceType: deviceType ? enumToStr(DeviceType, deviceType) : null}));
Device.getDeviceTypeAsync().then(deviceType => {
if (deviceType) {
setDeviceInfo(prev => ({...prev, deviceType: enumToStr(DeviceType, deviceType)}));
}

Copy link
Contributor

@gwainor gwainor left a comment

Choose a reason for hiding this comment

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

LFTM

jontsai
jontsai previously approved these changes Oct 16, 2024
Copy link
Contributor

@gwainor gwainor left a comment

Choose a reason for hiding this comment

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

LFTM

@jontsai jontsai merged commit da7e3ae into hacktoolkit:master Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants