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

Rename "Element" to "Content" #150

Merged
merged 4 commits into from
May 29, 2020
Merged

Rename "Element" to "Content" #150

merged 4 commits into from
May 29, 2020

Conversation

kyleve
Copy link
Collaborator

@kyleve kyleve commented May 29, 2020

The naming of ItemElement, HeaderFooterElement was a bit confusing, since it didn't really convey how it was different from Item or HeaderFooter, and also made talking about Blueprint integration confusing, because Blueprint refers to its content as Element. Thus, rename to ItemContent and HeaderFooterContent, plus update all relevant code, docs, comments, etc.

@kyleve kyleve requested a review from kylebshr May 29, 2020 02:11
…ecomes 'ItemContent', and 'HeaderFooterElement' becomes 'HeaderFooterContent'.
@kyleve kyleve force-pushed the kve/element-to-content branch from 68a90e1 to 332666a Compare May 29, 2020 02:13
@@ -1,31 +0,0 @@
//
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved into mainline.

{
//
// MARK: Creating Blueprint Element Representations
//

var element : BlueprintUI.Element { get }
var elementRepresentation : Element { get }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed this to line up with ProxyElement, so if you make a ProxyElement conform to BlueprintHeaderFooterContent, you don't also need an element property.



public struct ListItemElement : ItemElement
public struct EmbeddedList : ItemContent
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 name seemed a bit clearer, so went with this instead of ListItemContent.

@@ -90,3 +80,19 @@ public struct ListItemElement : ItemElement
ListView(frame: frame)
}
}

public extension EmbeddedList
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nested ListItemSizing into EmbeddedList.

@@ -209,8 +209,18 @@ public extension ItemElement where Self:Equatable
}


/// Provides a default implementation of `identifier` when self conforms to Swift's `Identifiable` protocol.
@available(iOS 13.0, *)
public extension ItemContent where Self:Identifiable
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this as well for easier Identifiable usage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think #if swift(>=5.1) is more correct here - Identifiable should be available anywhere as long as it's Swift 5.1 right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh yeah, maybe – let me try that; Xcode told me to add the iOS 13 check so I did

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, Xcode doesn't like it:

#if swift(>=5.1)
public extension ItemContent where Self:Identifiable
{
    var identifier : Identifier<Self> {
        .init(self.id)
    }
}
#endif

Still asks me to do:

/Users/k/Desktop/Development/Listable3/Listable/Sources/Item/ItemContent.swift:214:41: 'Identifiable' is only available in iOS 13 or newer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dang, the docs didn't mention an iOS version so I thought it would work. Oh well!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Might be because it was added in 5.1, which IIRC was the first ABI stable release, so it's now baked into iOS vs. embedded in the bin?

{
lhs.add(rhs)
}

public static func += <Content:ItemContent>(lhs : inout Section, rhs : Content)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These came in from BlueprintLists.

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.

Might not be worth it, but could make migration easier if you added type aliases for public renamed types and deprecated them. Something like:

@available(*, deprecated, renamed: "ItemContent")
typealias ItemElement = ItemContent

@@ -209,8 +209,18 @@ public extension ItemElement where Self:Equatable
}


/// Provides a default implementation of `identifier` when self conforms to Swift's `Identifiable` protocol.
@available(iOS 13.0, *)
public extension ItemContent where Self:Identifiable
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think #if swift(>=5.1) is more correct here - Identifiable should be available anywhere as long as it's Swift 5.1 right?

@kyleve
Copy link
Collaborator Author

kyleve commented May 29, 2020

Might not be worth it, but could make migration easier if you added type aliases for public renamed types and deprecated them. Something like:

@available(*, deprecated, renamed: "ItemContent")
typealias ItemElement = ItemContent

I'd thought about doing this, but figured since we'll also be doing the bumping in SPOS, it wouldnt make a ton of diference – though I guess it could help people find the new APIs – thoughts?

@kylebshr
Copy link
Collaborator

I know we're pre 1.0 but there's definitely consumers outside of SPOS already (i.e., that hacker news demo app, other side projects) so it'd be nice I think.

@kyleve
Copy link
Collaborator Author

kyleve commented May 29, 2020

Good point! Will do

@kyleve kyleve merged commit f437520 into master May 29, 2020
@kyleve kyleve deleted the kve/element-to-content branch May 29, 2020 22:26
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.

2 participants