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

feat: added bowser to detect mobile or desktop #52

Merged
merged 5 commits into from
Apr 10, 2024
Merged

Conversation

malmen237
Copy link
Contributor

No description provided.

@malmen237 malmen237 requested a review from martinstark April 9, 2024 15:05
Copy link
Collaborator

@martinstark martinstark left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of questions

src/bowser.ts Outdated Show resolved Hide resolved
Comment on lines 68 to 71
// TODO remove this after testing
console.log(`Is this a mobile? "${isMobile}"`);
console.log(`Is this a desktop? "${isDesktop}"`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tested that the booleans are set properly at this point? You could test it on mobile by doing some conditional rendering based on if isMobile is true

Copy link
Contributor Author

@malmen237 malmen237 Apr 10, 2024

Choose a reason for hiding this comment

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

Have you tested that the booleans are set properly at this point? You could test it on mobile by doing some conditional rendering based on if isMobile is true

Tested on chrome browser, where i switched view in dev mode and tested on x-code simulator to test apple-devices, worked in both.
But isn't great at detecting tablet. Only ipad mini is detected as tablet the bigger screens are detected as desktop.

@malmen237 malmen237 requested a review from martinstark April 10, 2024 10:27
Copy link
Collaborator

@martinstark martinstark left a comment

Choose a reason for hiding this comment

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

Good stuff! What do you think about having only isMobile and aim to have two designs (mobile specific, and all other devices)?

The more booleans we have, the more complex the support matrix becomes. E.g. I'd rather check if (!isMobile) than have to check if (isDesktop || isTablet).

If you agree, I think we can have only isMobile, remove the console logs, and then merge!

@malmen237
Copy link
Contributor Author

Good stuff! What do you think about having only isMobile and aim to have two designs (mobile specific, and all other devices)?

The more booleans we have, the more complex the support matrix becomes. E.g. I'd rather check if (!isMobile) than have to check if (isDesktop || isTablet).

If you agree, I think we can have only isMobile, remove the console logs, and then merge!

My thought exactly, as long as we just want to restrict just the mobile-experience there is no need for the other checks.

@malmen237 malmen237 merged commit 72707b2 into main Apr 10, 2024
2 checks passed
@malmen237 malmen237 deleted the feat/add-bowser branch April 10, 2024 13:41
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.

2 participants