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

[WIP] Release/5.0.0 #5028

Merged
merged 25 commits into from
Jun 8, 2023
Merged

[WIP] Release/5.0.0 #5028

merged 25 commits into from
Jun 8, 2023

Conversation

pmairoldi
Copy link
Collaborator

Work to release a 5.0 version of Charts. The main goal of the release is to rename the library to DGCharts.

@pmairoldi pmairoldi added this to the 5.0.0 milestone Mar 21, 2023
@liuxuan30
Copy link
Member

liuxuan30 commented Mar 23, 2023

@liuxuan30
Copy link
Member

btw, have you tried running the tests, should we update the test images using new Xcode?
Previously I ran into failures because the UIKit update something and the images had like 0.1% or 1% diff, mostly like the font is a little bit bolder or slightly position shift.

@pmairoldi
Copy link
Collaborator Author

tried ChartsDemo-iOS, not build when BUILD_LIBRARY_FOR_DISTRIBUTION is on
I have filed a post https://forums.swift.org/t/bug-report-unable-to-build-demo-project-if-imported-swift-framework-turned-on-build-library-for-distribution/63904

Yeah this is the issue I ran into as well.

@pmairoldi
Copy link
Collaborator Author

btw, have you tried running the tests, should we update the test images using new Xcode?
Previously I ran into failures because the UIKit update something and the images had like 0.1% or 1% diff, mostly like the font is a little bit bolder or slightly position shift.

Hmm yeah I don’t know. The test run fine on CI but it might be a good idea. Could you add an issue to the 5.0 milestone so we don’t forget.

@liuxuan30
Copy link
Member

liuxuan30 commented Mar 24, 2023

btw, have you tried running the tests, should we update the test images using new Xcode?
Previously I ran into failures because the UIKit update something and the images had like 0.1% or 1% diff, mostly like the font is a little bit bolder or slightly position shift.

Hmm yeah I don’t know. The test run fine on CI but it might be a good idea. Could you add an issue to the 5.0 milestone so we don’t forget.

#5033.

I do see
Swift / iOS (OS=16.0,name=iPhone 14 Pro) (pull_request)

Successful in 8m ran 66 tests and passed.

On my mac it is

 Executed 66 tests, with 52 failures (0 unexpected) in 12.944 (13.016) seconds
Program ended with exit code: 1, 

Xcode Version 14.2 (14C18), 14 Pro Simulator 16.2.

Compared two diffs, mostly the label diff, the issue is the label I generated is grey, while the test image is black
It's weird I thouched nothing and the color has huge difference. I didn't specify grey.

image
image

@rineek
Copy link

rineek commented Mar 29, 2023

On my mac it is

 Executed 66 tests, with 52 failures (0 unexpected) in 12.944 (13.016) seconds
Program ended with exit code: 1, 

Xcode Version 14.2 (14C18), 14 Pro Simulator 16.2.

Compared two diffs, mostly the label diff, the issue is the label I generated is grey, while the test image is black It's weird I thouched nothing and the color has huge difference. I didn't specify grey.

image image

Toggle Appearance option for iOS simulator changes test results:
Dark:
Screenshot 2023-03-29 at 19 08 23
Light:
Screenshot 2023-03-29 at 19 09 43
Xcode 14.2 (14C18), 14 Pro Simulator 16.2

@liuxuan30
Copy link
Member

tried ChartsDemo-iOS, not build when BUILD_LIBRARY_FOR_DISTRIBUTION is on I have filed a post https://forums.swift.org/t/bug-report-unable-to-build-demo-project-if-imported-swift-framework-turned-on-build-library-for-distribution/63904

radar: https://feedbackassistant.apple.com/feedback/12078082

I didn't get any reply from Swift forum nor radar. Not sure what we should go for, either wait or make a release but pointing out the Objc'demo is broken

@liuxuan30
Copy link
Member

On my mac it is

 Executed 66 tests, with 52 failures (0 unexpected) in 12.944 (13.016) seconds
Program ended with exit code: 1, 

Xcode Version 14.2 (14C18), 14 Pro Simulator 16.2.
Compared two diffs, mostly the label diff, the issue is the label I generated is grey, while the test image is black It's weird I thouched nothing and the color has huge difference. I didn't specify grey.
image image

Toggle Appearance option for iOS simulator changes test results: Dark: Screenshot 2023-03-29 at 19 08 23 Light: Screenshot 2023-03-29 at 19 09 43 Xcode 14.2 (14C18), 14 Pro Simulator 16.2

it shouldn't bother? or the dark mode changed the label color or background color? Normally we don't care dark or not. Just make sure the chart positions itself correctly.

@rineek
Copy link

rineek commented Apr 12, 2023

On my mac it is

 Executed 66 tests, with 52 failures (0 unexpected) in 12.944 (13.016) seconds
Program ended with exit code: 1, 

Xcode Version 14.2 (14C18), 14 Pro Simulator 16.2.
Compared two diffs, mostly the label diff, the issue is the label I generated is grey, while the test image is black It's weird I thouched nothing and the color has huge difference. I didn't specify grey.
image image

Toggle Appearance option for iOS simulator changes test results: Dark: Screenshot 2023-03-29 at 19 08 23 Light: Screenshot 2023-03-29 at 19 09 43 Xcode 14.2 (14C18), 14 Pro Simulator 16.2

it shouldn't bother? or the dark mode changed the label color or background color? Normally we don't care dark or not. Just make sure the chart positions itself correctly.

In dark mode where only BarChartTests failed, simulator labels' colors were white, but chart positions remained the same:
Screenshot 2023-04-12 at 15 37 51
Snapshot images for Logical width 393 and Logical height 852(e.g. iPhone 14 Pro) have different label colors for different chart types:
Screenshot 2023-04-12 at 15 48 56
Screenshot 2023-04-12 at 15 49 04
If I update BarChartTests for that logical size with white label snapshots then all tests pass:
Screenshot 2023-04-12 at 16 12 48

@liuxuan30
Copy link
Member

@rineek usually we don't care too much about dark mode tests.

@waterskier2007
Copy link

Any update here? Who is the right person to make the call as to whether or not these test cases actually matter?

@pmairoldi
Copy link
Collaborator Author

Any update here? Who is the right person to make the call as to whether or not these test cases actually matter?

Hey they don’t actually matter to release. I think we can just force light mode in the tests. The real problem is #5028 (comment)

It is a blocker. If you have ideas I’m open to them :p

@FelixHerrmann
Copy link
Contributor

FelixHerrmann commented May 6, 2023

tried ChartsDemo-iOS, not build when BUILD_LIBRARY_FOR_DISTRIBUTION is on I have filed a post https://forums.swift.org/t/bug-report-unable-to-build-demo-project-if-imported-swift-framework-turned-on-build-library-for-distribution/63904

radar: https://feedbackassistant.apple.com/feedback/12078082

Hey guys, bumping the minimum iOS version of the ChartsDemo-iOS target to iOS 13 resolves the issue. I've actually created a fresh Objective-C project, added the binary framework, created the Swift files to use the marker subclasses and as soon as I lowered the minimum deployment target to < 13.0 the build error appear. Hope that helps and we can get a release soon.

@FelixHerrmann
Copy link
Contributor

Also, can we add #5041 and #5043 to this release? That would be awesome!

@pmairoldi
Copy link
Collaborator Author

@FelixHerrmann thanks. Will look at all that maybe this weekend.

@waterskier2007
Copy link

@pmairoldi do you think it's reasonable to bump the minimum version as @FelixHerrmann mentioned?

@pmairoldi
Copy link
Collaborator Author

I haven’t tried yet.

@waterskier2007
Copy link

@pmairoldi is there any way I could assist in this. Is a new PR required just to bump the min version and test?

@waterskier2007
Copy link

#5062

Brendan Kirchner and others added 3 commits May 30, 2023 10:38
@waterskier2007
Copy link

@pmairoldi any update on when we might be able to get a release?

@liuxuan30
Copy link
Member

liuxuan30 commented Jun 7, 2023

so, seems good to go? just try to compare the light mode tests?

@pmairoldi
Copy link
Collaborator Author

There are a couple points left in milestone 5.0.0

@liuxuan30
Copy link
Member

eh. I will see what I could take on tomorrow

@pmairoldi pmairoldi merged commit 0a229f8 into master Jun 8, 2023
8 checks passed
@pmairoldi pmairoldi deleted the release/5.0.0 branch June 8, 2023 20:50
@pmairoldi
Copy link
Collaborator Author

5.0.0 has been released.

@liuxuan30
Copy link
Member

cheers, maybe get a beer from the funds!

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.

None yet

8 participants