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.

Sign up for GitHub

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

Jump to bottom

feat: wire cells upload images & videos previews - WPB-15844 #2513

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

feat: wire cells upload images & videos previews - WPB-15844 #2513

wants to merge 15 commits into from

Conversation

El-Fitz
Copy link
Contributor

@El-Fitz El-Fitz commented Feb 11, 2025 •
edited by github-actions bot
Loading

TaskWPB-15844 [iOS] Create upload preview component for images

Issue

Implements upload previews for image (WPB-15844) and videos (WPB-15845).
Accessibility labels will be added when available.

Testing

See UI Tests snapshots.

Checklist

  • Title contains a reference JIRA issue number like [WPB-XXX].
  • Description is filled and free of optional paragraphs.
  • Adds/updates automated tests.

UI accessibility checklist

If your PR includes UI changes, please utilize this checklist:

  • Make sure you use the API for UI elements that support large fonts.
  • All colors are taken from WireDesign.ColorTheme or constructed using WireDesign.BaseColorPalette.

@El-Fitz El-Fitz self-assigned this Feb 11, 2025
@El-Fitz El-Fitz force-pushed the feat/wire-cells-upload-preview-images branch from 0f8aaab to 903d410 Compare February 11, 2025 13:12
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 11, 2025 •
edited
Loading

Test Results

  1 files   10 suites   56s ⏱️
133 tests 131 ✅ 1 💤 1 ❌
133 runs  132 ✅ 1 💤 0 ❌

For more details on these failures, see this check.

Results for commit ebd2bd3.

♻️ This comment has been updated with latest results.

caldrian
caldrian reviewed Feb 11, 2025
View reviewed changes
Copy link
Contributor

@caldrian caldrian left a comment

Choose a reason for hiding this comment

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

I think we should have the schemes WireDebugAll and WireCellsAll, which CI will use to run tests.
I can do that, if you prefer.

WireCells/Package.swift
]
),
]
)

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have the upcoming features enabled, like other packages do, see WireFoundation please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This package builds with the 6.0 toolchain, so those are not upcoming features and are already enabled for this package.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree:
ExistentialAny
InternalImportsByDefault
FullTypedThrows

Copy link
Contributor

Choose a reason for hiding this comment

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

made the changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we should update WireAuthentication/Package.swift in a separate PR

for target in package.targets {
    if target.isTest {
        target.dependencies += [WireTestingPackage]
    }
    target.swiftSettings = (target.swiftSettings ?? []) + [
        // TODO: [WPB-15967] Enable `ExistentialAny` upcoming feature
        .enableUpcomingFeature("GlobalConcurrency"),
        .enableExperimentalFeature("StrictConcurrency"),
        .unsafeFlags(["-enable-bare-slash-regex"]) // For regex literals
    ]
}

WireDebug/Package.swift Outdated Show resolved Hide resolved
@El-Fitz
Copy link
Contributor Author

El-Fitz commented Feb 11, 2025

I think we should have the schemes WireDebugAll and WireCellsAll, which CI will use to run tests. I can do that, if you prefer.

Oh. Yes, please do. But it doesn’t make sense for WireDebug.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Feb 11, 2025 •
edited
Loading

Datadog Report

Branch report: feat/wire-cells-upload-preview-images
Commit report: 855cce7
Test service: wire-ios-mono

✅ 0 Failed, 98 Passed, 1 Skipped, 39.86s Total Time

samwyndham
samwyndham requested changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@samwyndham samwyndham left a comment

Choose a reason for hiding this comment

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

Looking good! I think it would make sense for the snapshots to include the dismiss button

WireCells/Tests/WireCellsUITests/Components/UploadVideoPreviewTests.swift
override func tearDown() {
snapshotHelper = nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): Add a snapshot test to show the different duration format variations

...Resources/ReferenceImages/UploadImagePreviewTests/testColorSchemeVariantsEmptyState.dark.png Outdated Show resolved Hide resolved
netbe
netbe requested changes Feb 12, 2025
View reviewed changes
Copy link
Collaborator

@netbe netbe left a comment

Choose a reason for hiding this comment

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

I left some comments about the setup mostly

fastlane/framework.rb Show resolved Hide resolved
wire-ios-build-assets Outdated Show resolved Hide resolved
...ReferenceImages/UploadImagePreviewTests/testDynamicTypeVariantsEmptyState.accessibility5.png Outdated Show resolved Hide resolved
@El-Fitz El-Fitz force-pushed the feat/wire-cells-upload-preview-images branch from 2536775 to becf032 Compare February 12, 2025 16:10
@El-Fitz El-Fitz requested a review from netbe February 12, 2025 16:10
netbe
netbe requested changes Feb 12, 2025
View reviewed changes
Copy link
Collaborator

@netbe netbe left a comment

Choose a reason for hiding this comment

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

the submodule should be reverted to a1c04f

...Resources/ReferenceImages/UploadVideoPreviewTests/testColorSchemeVariantsEmptyState.dark.png Outdated Show resolved Hide resolved
wire-ios-build-assets Outdated Show resolved Hide resolved
@El-Fitz El-Fitz requested a review from netbe February 12, 2025 19:42
@netbe netbe mentioned this pull request Feb 12, 2025
Open
5 tasks
netbe
netbe approved these changes Feb 12, 2025
View reviewed changes
Copy link
Collaborator

@netbe netbe left a comment

Choose a reason for hiding this comment

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

🚢

@El-Fitz El-Fitz force-pushed the feat/wire-cells-upload-preview-images branch from f95930d to 90d1cd7 Compare February 13, 2025 08:49
caldrian
caldrian approved these changes Feb 13, 2025
View reviewed changes
WireCells/Sources/WireCellsImplementation/WireCellsServiceImplementation.swift
Comment on lines -117 to +125
let request = RestLookupRequest()
var request = RestLookupRequest()
let locator = RestNodeLocator(path: path)
request.locators = RestNodeLocators(many: [locator])
let requestBuilder = NodeServiceAPI.lookupWithRequestBuilder(body: request, apiConfiguration: cellsApiConfig)

do {
let response: Response<RestNodeCollection> = try await requestBuilder.execute()
return response.body.nodes
return response.body.nodes ?? []
Copy link
Contributor

Choose a reason for hiding this comment

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

There were build errors which I fixed, please verify @El-Fitz

Copy link
Contributor Author

@El-Fitz El-Fitz Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, probably due to the change in the dependency version. They moved from a class to a struct. Thank you.

WireCells/Package.swift
]
),
]
)

Copy link
Contributor

Choose a reason for hiding this comment

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

made the changes

caldrian
caldrian reviewed Feb 14, 2025
View reviewed changes
WireCells/Tests/TestPlans/AllTests.xctestplan
@@ -15,6 +15,13 @@
},
"testTargets" : [
{
"skippedTests" : [
"WireCellsTests",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are the tests skipped?
How can we prevent to forget about them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/// These tests are intended for locally testing the service, not for CI.

caldrian
caldrian reviewed Feb 14, 2025
View reviewed changes
fastlane/framework.rb Outdated
Comment on lines 154 to 155
when "WireCellsAll"
name
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
when "WireCellsAll"
name
when "WireCells"
"WireCellsAll"
when "WireDebug"
"WireDebugAll"

Copy link
Contributor Author

@El-Fitz El-Fitz Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks!

For WireDebug, there are no WireDebug tests, and they wouldn't make sense.
Unless we're going to make snapshots & unit tests for our debug menu?

@El-Fitz El-Fitz force-pushed the feat/wire-cells-upload-preview-images branch from 5b82dee to ebd2bd3 Compare February 14, 2025 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caldrian caldrian caldrian approved these changes

@netbe netbe netbe approved these changes

@samwyndham samwyndham Awaiting requested review from samwyndham

Requested changes must be addressed to merge this pull request.

Assignees

@El-Fitz El-Fitz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@El-Fitz @netbe @samwyndham @caldrian