Added back button for nav bar #141

stefanrenne · 2020-11-04T15:58:09Z

What's in this PR?

Replace this text with information about what's in your PR. What's changed? Why has it changed? Reference any GitHub issue this PR relates to, etc.

Added back button in nav bar

Pre-merge checklist

Before merging any PR, please check the following common things that should be done beforehand. These aren't all always required, so just check the box if it doesn't apply.

When adding files, make sure they're added to the right target. If you're adding new files that should be bundled up with Cocoapods etc, they need to be added to the TABTestKit target, not Pods-TABTestKit_Example etc.
Run pod install to ensure that the latest changes are in the Example project. Without this, Carthage might not see the latest changes.
Added and updated tests where possible. This isn't always possible but try wherever you can. The example app contains UI tests to test many of the TABTestKit features.
Updated the CHANGELOG. For any changes pending a release, add to the Pending section. For releases, move everything pending to the release section.
Updated the README. Add info for any new features, update existing info for anything that's changed or needs extra info.

stefanrenne · 2020-11-04T15:59:20Z

A different solution would be to add a custom Element, don't know what would be preferred?

Example:

struct BackBarButton: Element, Tappable {
    let id: String? = nil
    let index: Int = 0
    let parent: Element = NavBar()
    let type: XCUIElement.ElementType = .button
}

KaneCheshire · 2020-11-04T16:17:53Z

A different solution would be to add a custom Element, don't know what would be preferred?

Example:
struct BackBarButton: Element, Tappable {
    let id: String? = nil
    let index: Int = 0
    let parent: Element = NavBar()
    let type: XCUIElement.ElementType = .button
}

I personally prefer this because it means you're still enforcing that all buttons should have some sort of ID, unlike if you allow nil as the id (which I hope helps with accessibility in the app because people will have to assign an accessibility label or identifier)

KaneCheshire · 2020-11-04T16:23:43Z

Don't most back buttons have a specific title or label? Either "Back" or the title of the previous screen? This could maybe be solved with an extension or update on NavBar:

extension NavBar {

  func backButton(titled title: String = "Back") -> Button {
    return Button(id: title, parent: self)
  }
}

stefanrenne · 2020-11-04T16:47:02Z

Don't most back buttons have a specific title or label? Either "Back" or the title of the previous screen? This could maybe be solved with an extension or update on NavBar:
extension NavBar {

  func backButton(titled title: String = "Back") -> Button {
    return Button(id: title, parent: self)
  }
}

By default the back item has the title of the previous screen, would be nice to make it more foolproof

print(navigationController!.navigationBar.backItem!)
// output: <UINavigationItem: 0x7ff8fb006180> title='sdgsdfgdf'

KaneCheshire · 2020-11-04T16:49:52Z

Don't most back buttons have a specific title or label? Either "Back" or the title of the previous screen? This could maybe be solved with an extension or update on NavBar:
extension NavBar {

  func backButton(titled title: String = "Back") -> Button {
    return Button(id: title, parent: self)
  }
}
By default the back item has the title of the previous screen, so I guess that won't work
print(navigationController!.navigationBar.backItem!)
// output: <UINavigationItem: 0x7ff8fb006180> title='sdgsdfgdf'

Yeah that's what I said 😬 in your tests you'd have to know what the previous screen was so you can tap the back button, but since you're in control of your tests that shouldn't be a problem?

Given(I: tap(navBar.backButton(titled: "sdgsdfgdf")))

stefanrenne · 2020-11-04T16:51:41Z

I have to check in our real project, because we are doing some magic to hide the title in the navigationbar.
Also the default "Back" could be language specific.
In our project the title seems to be empty

po navigationController?.navigationBar.backItem?.backBarButtonItem
▿ Optional<UIBarButtonItem>
  - some : <UIBarButtonItem: 0x7fa3ffc5d8c0> menuOnTouchDown menu=0x6000013bad50 title=''

KaneCheshire · 2020-11-04T17:03:07Z

I have to check in our real project, because we are doing some magic to hide the title in the navigationbar.
Also the default "Back" could be language specific.
In our project the title seems to be empty
po navigationController?.navigationBar.backItem?.backBarButtonItem
▿ Optional<UIBarButtonItem>
  - some : <UIBarButtonItem: 0x7fa3ffc5d8c0> menuOnTouchDown menu=0x6000013bad50 title=''

You could just remove the default, it was there as a suggestion :)

I'm guessing the title is "" because you've removed the title and just have an arrow? What happens for VoiceOver when it navigates over that, I assume it just reads out "Back"? It might be worth putting a breakpoint in your tests when the button is visible and then printing out the hierarchy so you can see what the tests can see:

po App.shared or po NavBar()

Alternatively you might be able to just use the Accessibility Inspector. As a last resort you could add a custom accessibilityIdentifier to the back button wherever you're removing the title?

stefanrenne · 2020-11-04T17:08:21Z

Yeah there you go, "Vorige" is Dutch for "Previous"

NavigationBar, 0x60000309a4c0, {{0.0, 58.0}, {414.0, 56.0}}, identifier: 'LargeTitleView'
   Button, 0x60000309a5a0, {{0.0, 64.0}, {44.0, 44.0}}, label: 'Vorige'

KaneCheshire · 2020-11-04T17:10:33Z

Yeah there you go, "Vorige" is Dutch for "Previous"

NavigationBar, 0x60000309a4c0, {{0.0, 58.0}, {414.0, 56.0}}, identifier: 'LargeTitleView'
   Button, 0x60000309a5a0, {{0.0, 64.0}, {44.0, 44.0}}, label: 'Vorige'

Cool so in your tests you could just use navBar.backButton(titled: "Vorige"), or you might be able to add an accessibilityIdentifier which would remain consistent between languages

This reverts commit e0ad7e9.

stefanrenne · 2020-11-04T17:15:24Z

Yeah there you go, "Vorige" is Dutch for "Previous"
NavigationBar, 0x60000309a4c0, {{0.0, 58.0}, {414.0, 56.0}}, identifier: 'LargeTitleView'
   Button, 0x60000309a5a0, {{0.0, 64.0}, {44.0, 44.0}}, label: 'Vorige'
Cool so in your tests you could just use navBar.backButton(titled: "Vorige"), or you might be able to add an accessibilityIdentifier which would remain consistent between languages

Sounds like a plan! I just reverted my previous commit & processed your feedback :)

theblixguy

Looks great! Could you add some tests as well and update the changelog + readme?

TABTestKit/Classes/Elements/NavBar.swift

CHANGELOG.md

KaneCheshire · 2020-11-09T10:29:22Z

Example/TABTestKit_ExampleUITests/TableSelectionScreen.swift

@@ -14,6 +14,5 @@ struct TableSelectionScreen: Screen {

    let trait = View(id: "TableSelection")
    let navBar = NavBar()
-    let backButton = Button(id: "Table")
-
+    let backButton = NavBar().backButton(titled: "Table")


Should this be navBar.backButton instead?

Suggested change

let backButton = NavBar().backButton(titled: "Table")

let backButton = navBar.backButton(titled: "Table")

TABTestKit/Classes/Elements/NavBar.swift

Added back button for nav bar

e0ad7e9

stefanrenne added 2 commits November 4, 2020 18:12

Revert "Added back button for nav bar"

3c13f9a

This reverts commit e0ad7e9.

processed feedback

708b03d

KaneCheshire linked an issue Nov 5, 2020 that may be closed by this pull request

Add back button support to NavBar #51

Open

theblixguy reviewed Nov 5, 2020

View reviewed changes

TABTestKit/Classes/Elements/NavBar.swift Outdated Show resolved Hide resolved

stefanrenne added 2 commits November 7, 2020 10:08

Updated changelog & specs

e16cf7e

processed feedback

7813a27

KaneCheshire reviewed Nov 9, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

KaneCheshire reviewed Nov 9, 2020

View reviewed changes

TABTestKit/Classes/Elements/NavBar.swift Outdated Show resolved Hide resolved

Processed feedback

a9c49f6

stefanrenne closed this Jul 25, 2023

stefanrenne deleted the feature/nav-back-button branch July 25, 2023 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added back button for nav bar #141

Added back button for nav bar #141

stefanrenne commented Nov 4, 2020 •

edited

Loading

stefanrenne commented Nov 4, 2020

KaneCheshire commented Nov 4, 2020

KaneCheshire commented Nov 4, 2020

stefanrenne commented Nov 4, 2020 •

edited

Loading

KaneCheshire commented Nov 4, 2020

stefanrenne commented Nov 4, 2020 •

edited

Loading

KaneCheshire commented Nov 4, 2020

stefanrenne commented Nov 4, 2020 •

edited

Loading

KaneCheshire commented Nov 4, 2020

stefanrenne commented Nov 4, 2020

theblixguy left a comment

KaneCheshire Nov 9, 2020

theblixguy Nov 9, 2020

	let backButton = NavBar().backButton(titled: "Table")
	let backButton = navBar.backButton(titled: "Table")

Added back button for nav bar #141

Added back button for nav bar #141

Conversation

stefanrenne commented Nov 4, 2020 • edited Loading

What's in this PR?

Pre-merge checklist

stefanrenne commented Nov 4, 2020

KaneCheshire commented Nov 4, 2020

KaneCheshire commented Nov 4, 2020

stefanrenne commented Nov 4, 2020 • edited Loading

KaneCheshire commented Nov 4, 2020

stefanrenne commented Nov 4, 2020 • edited Loading

KaneCheshire commented Nov 4, 2020

stefanrenne commented Nov 4, 2020 • edited Loading

KaneCheshire commented Nov 4, 2020

stefanrenne commented Nov 4, 2020

theblixguy left a comment

Choose a reason for hiding this comment

KaneCheshire Nov 9, 2020

Choose a reason for hiding this comment

theblixguy Nov 9, 2020

Choose a reason for hiding this comment

stefanrenne commented Nov 4, 2020 •

edited

Loading

stefanrenne commented Nov 4, 2020 •

edited

Loading

stefanrenne commented Nov 4, 2020 •

edited

Loading

stefanrenne commented Nov 4, 2020 •

edited

Loading