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

Activity Indicator Demo: Dynamic start/stop activity button title #1958

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

imthath-m
Copy link
Contributor

@imthath-m imthath-m commented Jan 26, 2024

Platforms Impacted

  • iOS
  • macOS

Description of changes

Updated title of action button in the settings of ActivityIndicator demo page. The title is now dynamic and changes based on the state of the activity indicators below.

In the iOS demo app, open ActivityIndicator. The button in the second section (Settings) will now read "Stop Activity". If you tap that, the activity indicators below will stop spinning (and also hide if that is enabled) and the button title will change to "Start Activity".

Previously the button was named "Start/Stop Activity" which did not exactly represent the state of the activity indicators below.

Binary change

No change in binary size as the change is only in the iOS demo app.

Verification

  • The button title is "Stop Activity" when activity indicators are running
  • The button title is "Start Activity" when activity indicators are stopped/hidden
Visual Verification

Before

CleanShot.2024-01-26.at.18.14.22.mp4

After

CleanShot.2024-01-26.at.16.51.59.mp4

Pull request checklist

This PR has considered:

  • Light and Dark appearances
  • iOS supported versions (all major versions greater than or equal current target deployment version)
  • VoiceOver and Keyboard Accessibility
  • Internationalization and Right to Left layouts
  • Different resolutions (1x, 2x, 3x)
  • Size classes and window sizes (iPhone vs iPad, notched devices, multitasking, different window sizes, etc)
  • iPad Pointer interaction
  • SwiftUI consumption (validation or new demo scenarios needed)
  • Objective-C exposure (provide it only if needed)
Microsoft Reviewers: Open in CodeFlow

@imthath-m imthath-m requested a review from a team as a code owner January 26, 2024 12:50
@imthath-m imthath-m force-pushed the activity-indicator-demo branch 2 times, most recently from 995c74d to 16c3f5b Compare February 3, 2024 14:31
@@ -235,6 +235,13 @@ class ActivityIndicatorDemoController: DemoTableViewController {

@objc private func startStopActivity() {
isAnimating.toggle()

guard let section: Int = ActivityIndicatorDemoSection.allCases.firstIndex(of: .settings),
let row: Int = ActivityIndicatorDemoSection.settings.rows.firstIndex(of: .startStopActivity) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we avoid declaring types for inline variables unless strictly needed. See the specific reference in our style guide at Type Inference | swift-guide (microsoft.github.io)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the linked guide, it would still make sense to declare types in such cases. Here Int is not directly used as literal. It is the result of a function firstIndex on another type. So for quick understanding, I think it is helpful to have the type Int specified here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The guide is consistent on this point:

Use type annotation as little as possible when declaring a constant or a variable; instead let the compiler infer the variable type.

Let the compiler infer the type of a constant or variable whenever possible.

The primary exception to this policy is for stored properties; local variables should almost never need a type declared, particularly when they are storing the result of a well-understood method like firstIndex.

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

2 participants