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

[Slider#204] Added snapshot tests + corrected CGColor not updating on traitCollectionDidChange + added comments on functions using CGLayers like applyShadow #677

Merged
merged 26 commits into from
Jan 10, 2024

Conversation

LouisBorleeAdevinta
Copy link
Contributor

From branch 204-component-slider-develop to 204-component-slider

private func _test(scenario: SliderScenario) {
let configurations = self.createConfigurations(from: scenario)
for configuration in configurations {
self.assertSnapshot(matching: configuration.view, modes: scenario.modes, sizes: [.medium], testName: configuration.testName)
Copy link
Contributor

Choose a reason for hiding this comment

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

We have some constant to have the same accessibility sizes for all components (ComponentSnapshotTestConstants). I think you can also put this value inside your configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

slider.widthAnchor.constraint(equalToConstant: 200)
])
switch value {
case .medium: slider.setValue(0.5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Because on the futur you will have the SwiftUI version, you can put the value (0.5, 0, 1) on your configuration ? Same for the testName.

You can check this configuration if you want: ProgressBarConfigurationSnapshotTests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -8,7 +8,7 @@

import UIKit

enum ComponentSnapshotTestMode: String {
enum ComponentSnapshotTestMode: String, CaseIterable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of CaseIterable, we manage the modes of the snapshot on a Constants:

  • ComponentSnapshotTestConstants.Modes.all if you want to test all values
  • ComponentSnapshotTestConstants.Modes.def if you want to test only the default value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@LouisBorleeAdevinta LouisBorleeAdevinta added Component Related to a component Draft It is still in progress labels Jan 5, 2024
@robergro
Copy link
Contributor

The code seems good to me !

For the demo, I don't know why but there is no spacing between checkbox.
simulator_screenshot_00610824-A805-43F2-B680-A8A79D46A89A

Also, when I a add a steps, sometime I have this kind of value:
Slider.valuedChanged: 0.8
Slider.valuedChanged: 0.90000004
Slider.valuedChanged: 1.0
Slider.valuedChanged: 0.90000004
Slider.valuedChanged: 0.8
I don't remember what we said about that

@LouisBorleeAdevinta
Copy link
Contributor Author

#677 (comment)
@robergro

About the checkbox spacing, it's just a stackview in which I didn't set spacing value, let me add that real quick :)

About the floating values, it's the thing with floating numbers https://stackoverflow.com/questions/588004/is-floating-point-math-broken
It's not something we should be worrying about and to be fair, I don't think people will have this case a lot

@LouisBorleeAdevinta LouisBorleeAdevinta merged commit 19c5e7d into 204-component-slider Jan 10, 2024
4 checks passed
@LouisBorleeAdevinta LouisBorleeAdevinta deleted the 204-component-slider-develop branch January 10, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component Related to a component Draft It is still in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants