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

ADDED open in app dialog popup #690

Open
wants to merge 3 commits into
base: archived-develop
Choose a base branch
from

Conversation

dhruv036
Copy link

@dhruv036 dhruv036 commented Oct 6, 2023

Issue Open RDS app from www-site

Issue:
Real-Dev-Squad/mobile-app#269

Changes:

  1. Added popup dialog in index.html and done some changes in index.js and main.css

Recording:

WhatsApp.Video.2023-10-06.at.2.43.44.PM.mp4

Copy link
Contributor

@shreya-mishra shreya-mishra left a comment

Choose a reason for hiding this comment

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

please add Test cases

js/index.js Outdated
window.onload = femo();

function femo() {
console.log('efd');
Copy link
Contributor

Choose a reason for hiding this comment

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

remove console log

js/index.js Outdated
let regexp = /android|iphone|kindle|ipad/i;
let details = navigator.userAgent;
let isMobileDevice = regexp.test(details);
window.onload = femo();
Copy link
Contributor

Choose a reason for hiding this comment

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

what's femo ?

@dhruv036
Copy link
Author

dhruv036 commented Oct 10, 2023

Refactored code and written test cases @shreya-mishra

Copy link
Member

@satyam73 satyam73 left a comment

Choose a reason for hiding this comment

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

Improve spacing one line spacing after function ends/in css

Comment on lines 33 to 34
});
test(`don't show dialog if not open in phone`, () => {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: add line spacing here

expect(result).toBe('true');
});
test(`don't show dialog if not open in phone`, () => {
const regexp = /Windows|macOS|/i; // for PC
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const regexp = /Windows|macOS|/i; // for PC
const regexp = /Windows|macOS|/i; // for desktop devices

js/index.js Outdated
Comment on lines 25 to 26
document.getElementById('okayBt').addEventListener('click', openApp);
document.getElementById('cancleBt').addEventListener('click', closeDialog);
Copy link
Member

Choose a reason for hiding this comment

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

don't use shorthands for naming that are hard to understand, also there is typo in cancel button

js/index.js Outdated
Comment on lines 58 to 70
var timeTaken = Date.now() - startTime;
if (timeTaken <= 1000) {
flag = true;
}
});

setTimeout(function () {
if (!flag) {
document.body.removeChild(iframe);
window.location.href = fallbackURL;
}
}, 1000); // Adjust the delay as needed
} else {
Copy link
Member

Choose a reason for hiding this comment

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

What's the intention of this part of the code?

Copy link
Author

@dhruv036 dhruv036 Oct 12, 2023

Choose a reason for hiding this comment

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

  1. This is for if time taken to come from rds mobile app is more then it means app is install.
  2. If flag is false means app doesn't install in phone and then it will re-direct to playstore(App mentioned in fallback url).

Copy link
Member

Choose a reason for hiding this comment

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

use const variables instead of let and var where ever it can be done

@satyam73
Copy link
Member

why there's so much changes in index.html file?

@satyam73
Copy link
Member

When you uninstalled why it is opening github app not rds mobille app?

@satyam73
Copy link
Member

With this PR if I am in android I have to mobile app regardless if I want to use RDS website?

@satyam73
Copy link
Member

Tests are failing please have a look

@dhruv036
Copy link
Author

why there's so much changes in index.html file?

Due to some replacement of code it is shows much changes here.

@dhruv036
Copy link
Author

Tests are failing please have a look

Resolved failing of test case.
Screenshot 2023-10-13 at 12 56 31 AM

@dhruv036
Copy link
Author

dhruv036 commented Oct 12, 2023

When you uninstalled why it is opening github app not rds mobille app?

It is for demo purpose.

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