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

[CM-1211] Add Accessibility #3

Merged
merged 8 commits into from
Mar 16, 2023
Merged

[CM-1211] Add Accessibility #3

merged 8 commits into from
Mar 16, 2023

Conversation

SahilSainiYML
Copy link
Contributor

@SahilSainiYML SahilSainiYML commented Mar 15, 2023 •
edited
Loading

Introduction

Added accessibility support to stepper.

Purpose

Accessibility and Dynamic text support.

📱 Screenshots

Stepper

📈 Coverage

Code

~97% code coverage.
Coverage

Documentation

100% documentation for public apis.

jazzy report

@SahilSainiYML
[UPDATE] Basic stepper with Accessibility
38a3aeb 
[UPDATE] change value on min max value update

[UPDATE]rebase conflict

[UPDATE] added test case for image testing

[UPDATE] resolve review comments

[UPDATE] file renamed

[UPDATE] image change and test cases update

[UPDATE] updated size according to string size

updated StepperControl+Appearance.swift comments
@SahilSainiYML
[UPDATE] typo corrected
7c0357a
@mpospese
Copy link
Contributor

No need to screenshot the linter in your PR description: CI/CD would catch that.

mpospese
mpospese reviewed Mar 15, 2023
View reviewed changes
Sources/YStepper/Assets/Strings/en.lproj/Localizable.strings Outdated Show resolved Hide resolved
Sources/YStepper/Assets/Strings/en.lproj/Localizable.strings Outdated Show resolved Hide resolved
Sources/YStepper/Enums/YStepper+Images.swift Outdated Show resolved Hide resolved
Sources/YStepper/Enums/YStepper+Strings.swift Outdated Show resolved Hide resolved
Sources/YStepper/Enums/YStepper+Images.swift Outdated Show resolved Hide resolved
Sources/YStepper/SwiftUI/Views/Stepper.swift Outdated Show resolved Hide resolved
Sources/YStepper/SwiftUI/Views/Stepper.swift Outdated Show resolved Hide resolved
Sources/YStepper/SwiftUI/Views/Stepper.swift Show resolved Hide resolved
Sources/YStepper/SwiftUI/Views/Stepper.swift Outdated Show resolved Hide resolved
Sources/YStepper/SwiftUI/Views/Stepper.swift Outdated Show resolved Hide resolved
@SahilSainiYML SahilSainiYML force-pushed the feature/CM-1211-AddAccessibility branch from bd8f782 to 0dee3e0 Compare March 15, 2023 12:19
@SahilSainiYML SahilSainiYML force-pushed the feature/CM-1211-AddAccessibility branch from 4e518a7 to 3621b0a Compare March 15, 2023 17:46
mpospese
mpospese approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@mpospese mpospese left a comment

Choose a reason for hiding this comment

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

Looks good. Do we want to insert a space between "current value" and "1" to help make sure it reads out correctly?

@mpospese
Copy link
Contributor

Not needed for this PR, but this code is duplicated and should be extracted into its own function (and covered in unit tests):

        if appearance.hasDeleteButton
            && value <= stepValue
            && minimumValue == 0 {

@SahilSainiYML SahilSainiYML merged commit c26cc76 into main Mar 16, 2023
@SahilSainiYML SahilSainiYML deleted the feature/CM-1211-AddAccessibility branch March 16, 2023 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpospese mpospese mpospese approved these changes

@devyml devyml Awaiting requested review from devyml

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@SahilSainiYML @mpospese