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

Add Daum app user agent #773

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Add Daum app user agent #773

merged 4 commits into from
Dec 12, 2024

Conversation

HyewonKkang
Copy link
Contributor

Prerequisites

Type of Change

enhancement

Description

  • Added support for recognizing the 'Daum' app in user agent strings.
  • AS-IS: 'Daum' app was not identified.
  • TO-BE: User agents with 'Daum' are now accurately parsed.

application download link: google play, apple apps

Test

  • Added tests for 'Daum' user agent string.
  • Ran the full test suite successfully.

Impact

Breaking Changes: None
User Actions: No changes required, backward compatibility maintained

Other Info

Copy link
Owner

@faisalman faisalman left a comment

Choose a reason for hiding this comment

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

Hi @HyewonKkang, Thank you for contributing to this project!

  • Please commit only the source files and tests. You can omit the .mjs files as they will be autogenerated.
  • The value in the Browser enum should match with the browser name in the result:
import { UAParser } from 'ua-parser-js';
import { Browser } from 'ua-parser-js/enums';

const result = UAParser(navigator.userAgent);

// string comparison must be the same
if (result.browser.name == Browser.DAUM) {
  console.log('You are using Daum app');
}

@HyewonKkang
Copy link
Contributor Author

@faisalman
Thank you for reviewing quickly.
I've applied the review comments, so please check.

Copy link
Owner

@faisalman faisalman left a comment

Choose a reason for hiding this comment

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

Hi, thanks for following up, but this issue still hasn't been addressed:

* The value in the `Browser` enum should match with the browser name in the result:
// string comparison must be the same
if (result.browser.name == Browser.DAUM) {

You can change the captured string into, for example:

/(daum)apps[\/ ]([\w\.]+)/i,

So both of them will match

@HyewonKkang
Copy link
Contributor Author

HyewonKkang commented Dec 12, 2024

@faisalman
Thanks for the great suggestions 😄

Copy link
Owner

@faisalman faisalman left a comment

Choose a reason for hiding this comment

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

Change the value in the test files as well and run npm test to make sure that the change is all good.

test/data/ua/browser/browser-all.json Outdated Show resolved Hide resolved
Copy link
Owner

@faisalman faisalman left a comment

Choose a reason for hiding this comment

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

All seems good now, thanks!

@faisalman faisalman merged commit 4601953 into faisalman:master Dec 12, 2024
5 checks passed
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.

3 participants