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

[device][ios] Added support for desktop detection #21636

Merged
merged 6 commits into from
Mar 14, 2023
Merged

[device][ios] Added support for desktop detection #21636

merged 6 commits into from
Mar 14, 2023

Conversation

robertherber
Copy link
Contributor

@robertherber robertherber commented Mar 9, 2023 •
edited
Loading

Why

On iOS added support for deviceType detection of Desktop on MacOS, checking for Catalyst and iPad app running on Mac.

Supersedes #15084 which got outdated before merging, broken apart as requested in previous review.

How

By checking the ProcessInfo.processInfo.isMacCatalystApp and ProcessInfo.processInfo.isiOSAppOnMac flags.

Test Plan

Tested in the bare-expo app. And used in production through patch-package.

Checklist

@robertherber robertherber changed the title added support for desktop detection on ios [expo-device] [ios] added support for desktop detection Mar 9, 2023
@expo-bot
Copy link
Collaborator

expo-bot commented Mar 9, 2023

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: SwiftLint Violations


Found 7 violations, 0 serious. See the review comments below for more insights.

Generated by ExpoBot 🤖 against a2cab8a

expo-bot
expo-bot reviewed Mar 9, 2023
View reviewed changes
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Mar 9, 2023
@robertherber robertherber mentioned this pull request Mar 9, 2023
Closed
3 tasks
fobos531
fobos531 suggested changes Mar 9, 2023
View reviewed changes
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
@robertherber
Copy link
Contributor Author

robertherber commented Mar 9, 2023 •
edited
Loading

@ide @tsapeta I'm all for linting - but getting linting feedback in CI is a recurring headscratcher when contributing to Expo packages. Building SwifLint locally feels is definitely a threshold to contributing - is there a simpler way?

@fobos531
Copy link
Contributor

fobos531 commented Mar 9, 2023

@ide @tsapeta I'm all for linting - but getting linting feedback in CI is a recurring headscratcher when contributing to Expo packages. Building SwifLint locally feels is definitely a threshold to contributing - is there a simpler way?

There's no need to build it locally. If you're on macOS (which I assume you are), there's a swiftlint homebrew formula which you can easily install. After that, position yourself in the packages/expo-device folder, run swiftlint --fix and that's all. Maybe this could be added to the contributing guide @tsapeta ?

@expo-bot
Copy link
Collaborator

expo-bot commented Mar 9, 2023

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: SwiftLint Violations


Found 20 violations, 0 serious. See the review comments below for more insights.

Generated by ExpoBot 🤖 against f86be6e

expo-bot
expo-bot reviewed Mar 9, 2023
View reviewed changes
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
@expo-bot
Copy link
Collaborator

expo-bot commented Mar 9, 2023

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: SwiftLint Violations


Found 5 violations, 0 serious. See the review comments below for more insights.

Generated by ExpoBot 🤖 against 07f17ea

expo-bot
expo-bot reviewed Mar 9, 2023
View reviewed changes
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
@expo-bot
Copy link
Collaborator

expo-bot commented Mar 9, 2023

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: SwiftLint Violations


Found 4 violations, 0 serious. See the review comments below for more insights.

Generated by ExpoBot 🤖 against 682e65d

expo-bot
expo-bot reviewed Mar 9, 2023
View reviewed changes
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
packages/expo-device/ios/DeviceModule.swift Outdated Show resolved Hide resolved
@robertherber
Copy link
Contributor Author

robertherber commented Mar 10, 2023 •
edited
Loading

@fobos531 @tsapeta @ide I've added #21654 with some notes about swiftlint in the contribution docs, I hope it can help other sporadic contributors as well, and help keep the CI greener.. :)

@tsapeta
Copy link
Member

tsapeta commented Mar 14, 2023

@robertherber I'm sorry that the bot and linter get you annoyed. Right now the number of comments it can include in the review is limited to 10, because GitHub's API counts every comment as a notification (even though they all are sent in one http request) which is the basis of rate-limiting mechanism. We'll keep working on improving the contributors experience though. Thanks for submitting the PR to the contribution docs 🙏

@tsapeta tsapeta changed the title [expo-device] [ios] added support for desktop detection [device][ios] Added support for desktop detection Mar 14, 2023
@tsapeta tsapeta merged commit 4426875 into expo:main Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@expo-bot expo-bot expo-bot left review comments

@fobos531 fobos531 fobos531 requested changes

@tsapeta tsapeta tsapeta approved these changes

@ide ide Awaiting requested review from ide ide is a code owner

Assignees
No one assigned
Labels
bot: suggestions ExpoBot has some suggestions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@robertherber @expo-bot @fobos531 @tsapeta