firefox-ios icon indicating copy to clipboard operation
firefox-ios copied to clipboard

Add FXIOS-9201 ⁃ Enhanced Tracking Protection certificates details screen

Open dicarobinho opened this issue 1 year ago • 8 comments

:scroll: Tickets

Jira ticket Github issue

:bulb: Description

Added Tracking Protection Certificates screen

:pencil: Checklist

You have to check all boxes before merging

  • [x] Filled in the above information (tickets numbers and description of your work)
  • [x] Updated the PR name to follow our PR naming guidelines
  • [ ] Wrote unit tests and/or ensured the tests suite is passing
  • [ ] When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • [ ] If needed, I updated documentation / comments for complex code and public methods
  • [ ] If needed, added a backport comment (example @Mergifyio backport release/v120)

dicarobinho avatar Aug 23 '24 09:08 dicarobinho

Messages
:book: Project coverage: 31.57%
:book: Edited 4 files
:book: Created 4 files

Client.app: Coverage: 30.27

File Coverage
TrackingProtectionViewController.swift 28.69% ⚠️
CertificatesHandler.swift 0.0% ⚠️
TrackingProtectionModel.swift 29.55% ⚠️
CertificatesViewModel.swift 71.43%
CertificatesViewController.swift 0.0% ⚠️
CertificatesCell.swift 0.0% ⚠️

Generated by :no_entry_sign: Danger Swift against fec4e51804a5c1ec31521cc2e55f4327969b8c7a

mobiletest-ci-bot avatar Aug 23 '24 09:08 mobiletest-ci-bot

This pull request has conflicts when rebasing. Could you fix it @dicarobinho? 🙏

mergify[bot] avatar Aug 26 '24 06:08 mergify[bot]

Sorry I didn't get a chance to review this on Friday @dicarobinho. Would you be able to provide instructions for how I can access this screen to test in the app? Thanks!

ih-codes avatar Aug 26 '24 15:08 ih-codes

Sorry I didn't get a chance to review this on Friday @dicarobinho. Would you be able to provide instructions for how I can access this screen to test in the app? Thanks!

No, at this step is not possible without doing some changes locally.

dicarobinho avatar Aug 27 '24 07:08 dicarobinho

@dicarobinho I haven't reviewed thoroughly yet, will give another look once @ih-codes comments are addressed. However, we will be using the certificate screen for the error pages as well. Is this view reusable in that case?

Please provide screenshots as well, it will help review. Thank you!

Screen is not done yet, so screenshots may not be useful at this step. (I have another TODO done for which I will create a PR after this one will be merged.)

dicarobinho avatar Aug 27 '24 09:08 dicarobinho

@dicarobinho I haven't reviewed thoroughly yet, will give another look once @ih-codes comments are addressed. However, we will be using the certificate screen for the error pages as well. Is this view reusable in that case? Please provide screenshots as well, it will help review. Thank you!

Screen is not done yet, so screenshots may not be useful at this step. (I have another TODO done for which I will create a PR after this one will be merged.)

Even if the screen is not done, it'll be helpful to add it to represent what code is being merged from a user perspective. At least from my side, it'll help me provide a quicker review to visualize. :)

cyndichin avatar Aug 27 '24 12:08 cyndichin

@dicarobinho I haven't reviewed thoroughly yet, will give another look once @ih-codes comments are addressed. However, we will be using the certificate screen for the error pages as well. Is this view reusable in that case? Please provide screenshots as well, it will help review. Thank you!

Screen is not done yet, so screenshots may not be useful at this step. (I have another TODO done for which I will create a PR after this one will be merged.)

Even if the screen is not done, it'll be helpful to add it to represent what code is being merged from a user perspective. At least from my side, it'll help me provide a quicker review to visualize. :)

Simulator Screenshot - iPhone 15 Pro Max - 2024-08-27 at 16 37 34

dicarobinho avatar Aug 27 '24 13:08 dicarobinho

Sorry I didn't get a chance to review this on Friday @dicarobinho. Would you be able to provide instructions for how I can access this screen to test in the app? Thanks!

No, at this step is not possible without doing some changes locally.

I don't mind making a few changes in my code so I can test the screen. But if it is complicated, then don't worry about it. The screenshot you posted helps! 👍 I imagine more detailed testing can be done once the screen is finished.

ih-codes avatar Aug 27 '24 21:08 ih-codes