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

[NT-206] Add accessibility label to the toggle #862

Merged
merged 4 commits into from
Oct 3, 2019

Conversation

dusi
Copy link
Contributor

@dusi dusi commented Sep 30, 2019

📲 What

As the title says

🤔 Why

The toggle wasn't very accessible since it didn't provide much context to the VoiceOver about what's being toggled ON/OFF

♿️ Accessibility

Navigate to Manage pledge screen

  • VoiceOver reads the reward received toggle as Reward received OFF, double tap to toggle setting if the toggle is OFF
  • VoiceOver reads the reward received toggle as Reward received ON, double tap to toggle setting if the toggle is ON

⏰ TODO

  • Localization will be done in a follow up PR

@dusi
Copy link
Contributor Author

dusi commented Sep 30, 2019

Unfortunately I didn't have a real device to build on so please test on a real device.

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

Verified AC on a device.

One thing I noticed is the label for the switch reads "Reward received, heading". Is that expected?

I'm also not sure whether to hold off approving until we have the strings.

@@ -14,5 +14,8 @@ final class ManageViewPledgeRewardReceivedViewController: ToggleViewController {

_ = self.toggle
|> checkoutSwitchControlStyle
|> \.accessibilityLabel %~ {
_ in localizedString(key: "Reward_received", defaultValue: "Reward received")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we still waiting for translations or can we use Strings.Reward_received? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried pulling new strings ... still not translated ... I thought we're not going to block PRs because of localizations but I'm happy to hold on for now

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't remember what we decided about localizations 😭but I think we should at least wait for the key to be available in the Strings file?

@cdolm92 cdolm92 removed their assignment Oct 1, 2019
@dusi
Copy link
Contributor Author

dusi commented Oct 1, 2019

I believe so..I was under the impression that these are section headers (please see the attachment)

Screen Shot 2019-10-01 at 3 58 55 PM

@dusi dusi added blocked a PR that is blocked for external reasons and removed needs review labels Oct 2, 2019
@dusi
Copy link
Contributor Author

dusi commented Oct 2, 2019

Waiting for base strings

@dusi
Copy link
Contributor Author

dusi commented Oct 2, 2019

Made https://github.com/kickstarter/kickstarter/pull/17573

Will wait for it to be deployed and I think we should not hold a11y by localization so I'll add a TODO to my follow up PR to address localization separately

@dusi dusi added needs review and removed blocked a PR that is blocked for external reasons labels Oct 2, 2019
@dusi
Copy link
Contributor Author

dusi commented Oct 2, 2019

@ifbarrera I've updated the strings (they're not localized yet)...but I think it should not hold this PR...I've also updated the ticket to include localization of reward received toggle logic in JIRA so that we can keep track of it and not sign off on it before it's localized.

@@ -406,6 +406,7 @@
"Reward_Surveys.zero" = "%{reward_survey_count} questionnaires";
"Reward_delivered" = "Récompense livrée ?";
"Reward_estimated_for_delivery_in_date" = "<b>Récompense</b> prévue pour %{delivery_date}";
"Reward_received" = "Récompense reçue";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol, actually already translated in 🇫🇷 🤦‍♂

@dusi dusi merged commit 0c417a8 into master Oct 3, 2019
@dusi dusi deleted the reward-received-toggle-a11y branch October 3, 2019 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants