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

Use Richer UI install for PWA on Chrome #10393

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ananyakaligal
Copy link
Contributor

Closes #8929

DRAFT PR - will edit this once all the tasks are achieved.

Stakeholders

@RayBB

@codecov-commenter
Copy link

codecov-commenter commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.31%. Comparing base (9c5cd67) to head (74ace62).
Report is 433 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10393      +/-   ##
==========================================
- Coverage   17.43%   17.31%   -0.13%     
==========================================
  Files          88       87       -1     
  Lines        4811     4835      +24     
  Branches      853      856       +3     
==========================================
- Hits          839      837       -2     
- Misses       3447     3471      +24     
- Partials      525      527       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ananyakaligal
Copy link
Contributor Author

@RayBB, I've added the desktop screenshots to the .mockup file and referenced the same images in the manifest.json. However, I'm not sure how to test this locally. Could you please help me with that?

@RayBB
Copy link
Collaborator

RayBB commented Jan 28, 2025

Can you please provide a link to the guides you've tried following already and what part you're getting suck at?
Even better if you could upload a short video of the install process and where it's going wrong.

@ananyakaligal
Copy link
Contributor Author

@RayBB I am sorry , it was a confusion.

I've added the screenshots to .mockup as of now.
We need to either extract the screenshot URLs dynamically from the .mockup file using a Python script and include them in manifest.json, or store the screenshots directly in static/images/ and update the manifest.json accordingly. I can implement the script for extraction if that’s the preferred approach. Which one would you prefer?

@RayBB
Copy link
Collaborator

RayBB commented Jan 30, 2025

I think storing them wow in the static folder is good!

@ananyakaligal
Copy link
Contributor Author

@RayBB works on desktop, couldnt test on mobile.

Should I also update .mockup with the new screenshots as @cdrini mentioned here #8929 (comment)

@RayBB
Copy link
Collaborator

RayBB commented Feb 2, 2025 via email

@ananyakaligal
Copy link
Contributor Author

@RayBB Done! updated both desktop and mobile screenshots.

@jimchamp jimchamp added the Needs: Response Issues which require feedback from lead label Feb 5, 2025
@RayBB RayBB marked this pull request as ready for review February 5, 2025 21:57
Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Good work on this one!

I think we prefer not to have the top bar that's part of the OS showing time and battery for mobile screenshots.
Can you take the screenshots using the devtools on your computer with "Responsive Design Mode"? Ideally I think they should be png as well so they're HD.
image

@ananyakaligal
Copy link
Contributor Author

ananyakaligal commented Feb 17, 2025

@RayBB Yes, I can do that

@ananyakaligal
Copy link
Contributor Author

@RayBB Changes made as requested. Is thsi good?

@RayBB
Copy link
Collaborator

RayBB commented Feb 18, 2025

Looks nice to me but I'll double check with staff this week!

@RayBB RayBB added Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. and removed Needs: Response Issues which require feedback from lead labels Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this.
Projects
Status: Waiting Review/Merge from Staff
Development

Successfully merging this pull request may close these issues.

Use Richer UI install for PWA on Chrome
5 participants