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 RDS app from my-site #506

Merged
merged 6 commits into from
Nov 7, 2023

Conversation

dhruv036
Copy link
Contributor

@dhruv036 dhruv036 commented Oct 20, 2023

Issue #501

Description:
This PR contains feature of open RDS mobile app through dialog whenever user open it in mobile browser. Note dialog will not open in desktop browser.

Is Development Tested?

  • YES
  • NO

1. Screen Recoding: Mobile
In case of mobile dialog will open and user can open app On click of okay if user has installed RDS it will open RDS app otherwise it will redirect to playstore for RDS app.

Mobile.Recording.mov

2. Screen Recoding: Desktop
In this case dialog will not open.

Screen.Recording.2023-10-28.at.6.41.59.PM.mov

Test Coverage

Screenshot 2023-10-28 at 6 46 41 PM Screenshot 2023-10-28 at 6 46 51 PM

@netlify
Copy link

netlify bot commented Oct 20, 2023

Deploy Preview for staging-my ready!

Name Link
🔨 Latest commit 18c8658
🔍 Latest deploy log https://app.netlify.com/sites/staging-my/deploys/6548ad62100a1a00072573f2
😎 Deploy Preview https://deploy-preview-506--staging-my.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dhruv036 dhruv036 marked this pull request as draft October 20, 2023 05:32
@shubhamsinghbundela
Copy link
Contributor

shubhamsinghbundela commented Oct 22, 2023

Can you please add video of your changes?

@dhruv036
Copy link
Contributor Author

Can you please add video of your changes?

Added

@dhruv036 dhruv036 marked this pull request as ready for review October 28, 2023 05:50
@Pratiyushkumar
Copy link

Can you please add video of your changes?

Added

can you please add a clear video, of the feature yo are trying to build is not clear

app/components/mobile-dialog.hbs Outdated Show resolved Hide resolved
app/components/mobile-dialog.hbs Outdated Show resolved Hide resolved
app/components/mobile-dialog.hbs Outdated Show resolved Hide resolved
app/components/mobile-dialog.js Show resolved Hide resolved
app/components/mobile-dialog.js Outdated Show resolved Hide resolved
app/components/mobile-dialog.js Outdated Show resolved Hide resolved
app/components/mobile-dialog.js Outdated Show resolved Hide resolved
app/styles/app.css Outdated Show resolved Hide resolved
app/styles/app.css Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

write proper tests

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added please check

Copy link

@ajoykumardas12 ajoykumardas12 left a comment

Choose a reason for hiding this comment

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

Are the colors used here are same with RDS design colors?
Also, pr description can be improved. Please add test coverage.

@satyam73
Copy link
Member

Tests are failing due to lint issues

@shubhamsinghbundela
Copy link
Contributor

  1. Screen Recoding: Mobile
    In case of mobile dialog will open and user can open app On click of okay if user has installed RDS it will open RDS app otherwise it will redirect to playstore for RDS app.

If app is not installed and when user click okay then it is not possible that it is directly redirect to that rds app to installed because in video i seen it redirect to github app.

if you don't know about above please ask once @shreya-mishra

@dhruv036
Copy link
Contributor Author

  1. Screen Recoding: Mobile
    In case of mobile dialog will open and user can open app On click of okay if user has installed RDS it will open RDS app otherwise it will redirect to playstore for RDS app.

If app is not installed and when user click okay then it is not possible that it is directly redirect to that rds app to installed because in video i seen it redirect to github app.

if you don't know about above please ask once @shreya-mishra

For demo purpose I have added github url as currently RDS app is not published in future it will be replace with RDS App.

@satyam73
Copy link
Member

satyam73 commented Oct 28, 2023

Please add mobile video here only without google drive link, also put this feature under feature flag

Copy link
Contributor

@shubhamsinghbundela shubhamsinghbundela left a comment

Choose a reason for hiding this comment

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

great work Dhruv...can you please check some of my comment

app/components/mobile-dialog.js Outdated Show resolved Hide resolved
app/templates/application.hbs Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

app/components/mobile-dialog.js Outdated Show resolved Hide resolved
app/components/mobile-dialog.js Show resolved Hide resolved
app/styles/app.css Outdated Show resolved Hide resolved
@dhruv036 dhruv036 closed this Oct 31, 2023
@dhruv036
Copy link
Contributor Author

Can you please add video of your changes?

Added

can you please add a clear video, of the feature yo are trying to build is not clear

@Pratiyushkumar I have added clear video with description please have a look.

@dhruv036 dhruv036 reopened this Oct 31, 2023
@dhruv036
Copy link
Contributor Author

dhruv036 commented Nov 1, 2023

Please add mobile video here only without google drive link, also put this feature under feature flag

Added

@dhruv036
Copy link
Contributor Author

dhruv036 commented Nov 1, 2023

Tests are failing due to lint issues

Resolved

app/components/mobile-dialog.hbs Outdated Show resolved Hide resolved
app/styles/app.css Outdated Show resolved Hide resolved
tests/integration/components/mobile-dialog-test.js Outdated Show resolved Hide resolved
app/styles/app.css Show resolved Hide resolved
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.

LGTM

@iamitprakash iamitprakash merged commit e8ad261 into Real-Dev-Squad:develop Nov 7, 2023
4 checks passed
@shubhamsinghbundela shubhamsinghbundela mentioned this pull request Nov 22, 2023
10 tasks
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.

6 participants