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

Improve using elements in lists #412

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

Conversation

kyleve
Copy link
Collaborator

@kyleve kyleve commented Jul 25, 2022

This PR changes the recommended way to convert Blueprint Elements into Items or HeaderFooters. The old way was to either manually define a BlueprintItemContent or BlueprintHeaderFooterContent, which worked great for more complex needs, or to use the ElementItem(...) or ElementHeaderFooter(...) funcs, which were meant for simpler use cases. However, the latter was still pretty wordy, and people would still regularly mess it up.

Here, we instead add extensions to Element to support both Item and HeaderFooter use cases: You can just directly return an Element anywhere an ItemContent or HeaderFooterContent was previously required (caveat: Where assignment happens directly; such as Section.header, you still need to do section.header = element.headerFooter()).

This PR also moves isEquivalent(to:) into its own interface (LayoutEquivalent), so (a) the docs can be shared, and (b) so elements can conform to it and be used in either the item or header/footer case.

TL;DR

Now that this PR exists, you can now do:

List {
   Section("some-id") {
       SomeElement()
       AnotherElement()
       SomeItemContent()
       MyCoolElement().listItem(id: "my-id") {
          $0.selectionStyle = .tappable
       }
   } header: {
      MyHeaderElement()
   } footer: {
      MyFooterElement().listHeaderFooter {
         ...
      }
   }
}

Checklist

Please do the following before merging:

  • Ensure any public-facing changes are reflected in the changelog. Include them in the Main section.

@@ -138,24 +138,6 @@ public struct Section
self.footer = footer()
}

/// Creates a new section with result builder-style APIs.
public init<IdentifierValue:Hashable>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was a pure dupe of the above, just with less parameters.

@kyleve kyleve force-pushed the kve/element-usage-improvements branch from 9a735a3 to 0435437 Compare July 25, 2022 03:59
@kyleve kyleve changed the title [WIP DNR] Improve using elements in lists Improve using elements in lists Jul 25, 2022
@kyleve kyleve marked this pull request as ready for review July 25, 2022 04:02
@kyleve kyleve force-pushed the kve/element-usage-improvements branch from f7c6499 to ca36edc Compare July 25, 2022 05:39
Copy link
Collaborator

@kylebshr kylebshr left a comment

Choose a reason for hiding this comment

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

I think some discussion points here would be:

a) Is this an even bigger foot gun than what we have now? Many people probably won't make their element equatable or IsEquivalentItem
b) Is there a performance concern from losing identifiers (most people won't add one)
c) Most elements have closures, which make equatability impossible. What's the correct thing to do there. Should there be more helpers for interacting with the row tap handlers, so that the element doesn't have to be re-applied for the closure to be correct?

BlueprintUILists/Sources/Element+Item.swift Outdated Show resolved Hide resolved
BlueprintUILists/Sources/Element+Item.swift Outdated Show resolved Hide resolved
@@ -0,0 +1,137 @@
//
// Element+Item.swift
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kylebshr Creating a thread to reply to stuff, so we can talk in a thread vs top level comments:

a) Is this an even bigger foot gun than what we have now? Many people probably won't make their element equatable or IsEquivalentItem

Hard to say – there's a bunch of existing ElementItem usages in POS now, and they're all pretty much wrong – the API was hard to use, so there's a lot of this:

ElementItem("formFields-element", id: \.self) { _, _ in
   // Return an element
}

ElementItem(transaction.token, id: \.self) { _, _ in ... }

ElementItem("error-state", id: \.self) { _, _ in ... }

ElementItem("business-organization-name-content", id: \.self) { _, _ in ... }

Etc...

But in this case the underlying value is "formFields-element", aka a static string; so isEquivalent always returns true, and the item will never re-size even when it should – which is already a foot gun and breaks things in weird ways.

b) Is there a performance concern from losing identifiers (most people won't add one)

Identifiers? No – the list smart enough to do a "best attempt" at creating stable identifiers when there's duplicate IDs (and identifiers are already salted with the ItemContent type), so even without explicitly provided IDs, you'll get stable IDs across updates. Eg, if this is your list, these will be the identifiers:

MyElement1() -> Identifier(MyElement1, 1)
MyElement2() -> Identifier(MyElement2, 1)
MyElement2() -> Identifier(MyElement2, 2)
MyElement1() -> Identifier(MyElement1, 2)

The main benefit to providing IDs is during mutative diffs, the list can more intelligently manage the changes.

c) Most elements have closures, which make equatability impossible. What's the correct thing to do there. Should there be more helpers for interacting with the row tap handlers, so that the element doesn't have to be re-applied for the closure to be correct?

Same thing as you'd do before with ItemContent / BlueprintItemContent; you need to manually implement Equatable or isEquivalent, and compare the equatable parameters that you can. There are some clever things we can do here to avoid needing this, but I didn't explore it: We can use mirror to find equatable parameters and compare them

@nononoah
Copy link
Collaborator

I agree with the core aspects of the direction here - the builders, the reversal of construction (from Item containing Element to Element modified to Item), and even some of the more controversial sugar that you've included. Thanks for backing this up with so much testing.

Copy link
Collaborator

@JaviSoto JaviSoto left a comment

Choose a reason for hiding this comment

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

Don't mind me, I was browsing Mastodon at 2am because I couldn't sleep and saw this PR and noticed a couple funny things 😄

ListableUI/Sources/IsEquivalent/CompareProperties.swift Outdated Show resolved Hide resolved
ListableUI/Sources/IsEquivalent/CompareProperties.swift Outdated Show resolved Hide resolved
ListableUI/Sources/IsEquivalent/CompareProperties.swift Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
public extension ListableArrayBuilder where ContentType == AnyItemConvertible {

/// Ensures that a well-formed error is presented when a non-Equatable or non-LayoutEquivalent element is provided.
@available(*, unavailable, message: "To be directly added to a List, an Element must conform to Equatable or LayoutEquivalent.")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • TODO: Test this is indeed causing an error before merging.

public extension ListableOptionalBuilder where ContentType == AnyHeaderFooterConvertible {

/// Ensures that a well-formed error is presented when a non-Equatable or non-LayoutEquivalent element is provided.
@available(*, unavailable, message: "To be directly added to a List, an Element must conform to Equatable or LayoutEquivalent.")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • TODO: Test this is indeed causing an error before merging.

…rovements

* origin/main: (123 commits)
  Update CHANGELOG.md (#508)
  Revert "Supplementary Tracking Fixes (#433)"
  Revert "Force layout before appear, to avoid animated updates (#505)"
  Force layout before appear, to avoid animated updates (#505)
  Update workaround versions (#506)
  Fix supplementary view + contained first responder reuse issue (#507)
  Supplementary Tracking Fixes (#433)
  Release 13.0.0 (#504)
  Update KeyboardObserver (#499)
  CONV-1435: Gravity layout frame change fix - Before: Layout gravity doesn't take into account frame changes. For example, when the orientation changes the scroll position (relative to the bottom) changes - After: Layout gravity takes frame changes into account so the when the frame changes the scroll position relative to the bottom remains unchanged
  Release 12.0.0 (#501)
  CONV-1435: Add scroll indicator insets to customScrollViewInsets (#500)
  CONV-1435: Gravity layout - Adds a new Chat App demo and a new behavior called verticalLayoutGravity.  When verticalLayoutGravity is set to bottom, scrolling works the way you would expect for a messaging app.
  expose onKeyboardFrameWillChange on ListProperties
  onKeyboardFrameWillChange: Improve CHANGELOG, DocC
  CONV-1435: Custom keyboard adjustment mode - Adds a .custom KeyboardAdjustmentMode to fully customize inset behavior
  remove contentOffset from isContentScrollable calculation, improve comment
  Add ListView#isContentScrollable property - Add this property to ListView. It will be used in conjunction with upcoming so-called gravity scrolling changes to workaround an animation issue with paging
  Update CI script to reference the `xcodesorg/made/xcodes` package for installing simulator runtimes. (#494)
  Swipe Action Updates (#489)
  ...
@robmaceachern robmaceachern marked this pull request as draft March 14, 2024 18:19
@kyleve kyleve marked this pull request as ready for review July 5, 2024 22:29
@kyleve
Copy link
Collaborator Author

kyleve commented Jul 5, 2024

Finally getting around to landing this!

…rovements

* origin/main: (33 commits)
  Prepare 14.3.0 (#540)
  Custom Update Animations (#539)
  Update version to 14.2.0
  Prepare 14.2.0 release, which contains a Blueprint update
  Bumping versions to 14.1.0 (#535)
  reorder control now proxies accessibility into a seperate element (#533)
  Prepare 14.0.3
  Fix a crash that could occur during cell reuse if a list contained different types of headers. The wrong ObjectIdentifier was being compared and stored.
  Get a repro for the reordering crash reported in #market-ios
  chore: Generated documentation now uses a static copyright notice to avoid noisy diffs (#530)
  Bumping versions to 15.0.2
  fix: Fix tap gesture swallowing touches in swipe actions view
  Bump to 14.0.1
  Fix SPM Blueprint dependency
  Release 14.0.0, update BlueprintUI to 3.0.0 (#525)
  chore: iOS 15 deployment target bump [UI-5187] (#524)
  chore: Bump CI to Xcode 15.1. Bump gems. [UI-5186] (#523)
  fix: don't cancel touches in view for tap gesture recognizer
  fix: don't cancel touches in view for tap gesture recognizer
  Revert weak change
  ...
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.

4 participants