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

Fix: issue 625 #716

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix: issue 625 #716

wants to merge 2 commits into from

Conversation

RV784
Copy link

@RV784 RV784 commented Mar 2, 2024

Issue Link -> #625

Code Changes

  • Added UIScreenshotService delegate to AppDelegate
  • Adding OBAListView instance from loadView() instead of adding it as a subview in viewDidLoad()

…ull screenshots across all screens with OBAListView
@CLAassistant
Copy link

CLAassistant commented Mar 2, 2024

CLA assistant check
All committers have signed the CLA.

@RV784
Copy link
Author

RV784 commented Mar 8, 2024

@aaronbrethorst Did you get the chance to review this PR?

Copy link
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR. In addition to the inline comments, here's what I've noted needs to change before this can be merged:

  1. Please remove .swiftpm/xcode/package.xcworkspace/contents.xcworkspacedata
  2. See screenshot below. This isn't working correctly.
  3. You'll need to get more clever with the searching for OBAListViewinstances, for example with the trip controller.

IMG_27D88C4015B8-2

@RV784
Copy link
Author

RV784 commented Mar 9, 2024

Hey @aaronbrethorst thanks for the feedbacks

For point 1 and 3 I'm working on this.
For second point can you please elaborate on the issue you are facing? Is it with screenshot functionality or the text is not clearly visible?

@aaronbrethorst
Copy link
Member

@RV784 the rounded cells have black backgrounds, rendering the text illegible. Compare to the product UI where the text on this screen is clearly visible.

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