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

Title stack #395

Merged
merged 5 commits into from
Aug 19, 2024
Merged

Title stack #395

merged 5 commits into from
Aug 19, 2024

Conversation

amegias
Copy link
Contributor

@amegias amegias commented Aug 9, 2024

🎟️ Jira ticket

https://jira.tid.es/browse/IOS-10494

πŸ₯… What's the goal?

We are going to work on https://jira.tid.es/browse/IOS-10327 where we refactor the UI of the soft/hard update feature.
Now, it needs the TitleView component but we implemented it as a UITableViewHeaderFooterView.
We need to split it.
https://www.figma.com/design/xNUwhH29z3Fre4foNltgtX/Novum_Specs?node-id=15589-86732
Captura de pantalla 2024-08-09 a las 11 06 24

🚧 How do we do it?

I've extracted the internal view to TitleView and rename the current one (which is a UITableViewHeaderFooterView) to TitleHeaderFooterView.
I found some bugs that I will mention in the code

πŸ§ͺ How can I verify this?

  1. Tests were not modified and they pass βœ…
  2. Demo
title.mov

Captura de pantalla 2024-08-09 a las 11 24 16


import UIKit

public class TitleHeaderFooterView: UITableViewHeaderFooterView {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the original component. It inherits from UITableViewHeaderFooterView but we need a UIView.
We expose this TitleHeaderFooterView which is a wrap of a TitleView inheriting from UITableViewHeaderFooterView

let style = Constants.styles[section]
headerView.title = style.title
headerView.style = style.titleStyle
headerView.linkTitle = style.linkTitle
headerView.onLinkLabelTapped = {
print("onLinkLabelTapped")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just to verify it's not broken

@@ -15,7 +15,7 @@ private enum ViewStyles {
static let minimumHeight: CGFloat = 40
}

public class TitleView: UITableViewHeaderFooterView {
public class TitleView: UIView {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what we need!

@@ -139,6 +132,7 @@ private extension TitleView {
func layoutViews() {
linkLabel.setContentHuggingPriority(.required, for: .horizontal)
linkLabel.setContentCompressionResistancePriority(.required, for: .horizontal)
linkLabel.isUserInteractionEnabled = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the link didn't work!!!!!!
I don't know if it's already broken in SW/iphoneapp (where it's being used)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that if it is working for them it is because they are using

titleView.isUserInteractionEnabled = true
titleView.addGestureRecognizer(UITapGestureRecognizer(target: self, action: #selector(linkLabelTapped)))

directly on the TitleView instance.


import UIKit

public class TitleHeaderFooterView: UITableViewHeaderFooterView {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know it's a breaking change but I think it should be....
Do you prefer keeping TitleView and call the internal view something like... TitleStack?? IMO it's worse but let me know.
I found occurrences in SmartWifi and iphoneapp, but it should be super easy to resolve them.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO I prefer the breaking change and fix in the project, should be no problem

Copy link
Contributor

Choose a reason for hiding this comment

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

I rather prefer the breaking change too.This won't be to difficult to update.

@amegias amegias marked this pull request as ready for review August 9, 2024 09:27
Copy link
Contributor

@DavidMarinCalleja DavidMarinCalleja left a comment

Choose a reason for hiding this comment

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


private lazy var titleView = TitleView()

public var onLinkLabelTapped: (() -> Void)? {
Copy link
Contributor

Choose a reason for hiding this comment

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

the get first and more consise version :

    public var onLinkLabelTapped: (() -> Void)? {
        get {  titleView.onLinkLabelTapped }
        set {  titleView.onLinkLabelTapped = newValue }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! thx!

@amegias amegias merged commit 161ac6d into main Aug 19, 2024
2 checks passed
@amegias amegias deleted the title-stack branch August 19, 2024 10:15
tuentisre pushed a commit that referenced this pull request Aug 19, 2024
# [32.0.0](v31.4.1...v32.0.0) (2024-08-19)

### Features

* **Title:** Split Title component and UIKit ([#395](#395)) ([161ac6d](161ac6d))

### BREAKING CHANGES

* **Title:** Rename TitleView with TitleHeaderFooterView. TitleHeaderFooterView will wrap the TitleView (Title component)

* Enable link interactions

* Extract Title component from TitleView (UITableViewHeaderFooterView)

* Rename and some fixes

* Rename tests path

* CR changes
@tuentisre
Copy link
Collaborator

πŸŽ‰ This PR is included in version 32.0.0 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants