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

Ios 9080 button chevron #294

Merged
merged 9 commits into from
Aug 18, 2023
Merged

Ios 9080 button chevron #294

merged 9 commits into from
Aug 18, 2023

Conversation

WanaldinoTelefonica
Copy link
Contributor

@WanaldinoTelefonica WanaldinoTelefonica commented Aug 8, 2023

🎟️ Jira ticket

IOS-9080

🥅 What's the goal?

  • Add a chevron image for linkButton

🚧 How do we do it?

  • For UIKit define a new property that configures an UIImageView with the corresponding image.

  • There is an enum with the posible cases with only chevron defined. This could be extended in the future.

  • For SwiftUI there is a new property withChevron on the ButtonStyle for misticaLink and misticaLinkInverse. This will add on MisticaButtonStyle the property .chevron on MisticaButton for rightImage

🧪 How can I verify this?

Check screenshots :D

🏑 AppCenter build

Use last version 25.4.0 (1)
https://install.appcenter.ms/orgs/tuenti-organization/apps/mistica-swiftui-ios/distribution_groups/public/releases/22

@WanaldinoTelefonica WanaldinoTelefonica requested a review from a team August 8, 2023 11:46
@WanaldinoTelefonica WanaldinoTelefonica self-assigned this Aug 8, 2023
@WanaldinoTelefonica WanaldinoTelefonica requested review from Gomay88 and alexanegon and removed request for a team August 8, 2023 11:46
@github-actions
Copy link

github-actions bot commented Aug 8, 2023

Screenshot tests report

✔️ All passing

@yceballost
Copy link
Contributor

please add @mistica-design as a reviewer :)

@WanaldinoTelefonica WanaldinoTelefonica requested review from a team, aweell, DavidMarinCalleja and amegias and removed request for a team, Gomay88 and alexanegon August 9, 2023 10:22
@amegias
Copy link
Contributor

amegias commented Aug 9, 2023

When the link isLoading, is the chevron hidden?

Copy link
Contributor

@amegias amegias left a comment

Choose a reason for hiding this comment

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

good work!

@yceballost
Copy link
Contributor

Can you generate a mistica catalog build to review it? I think you can deploy it with an action (confirm with @amegias)

Comment on lines +70 to +79
private var _image: UIImage {
switch self {
case .chevron: return .chevron
case let .custom(image: image): return image
}
}

var image: UIImage {
_image.withRenderingMode(.alwaysTemplate)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have image: UIImage and _image: UIImage?

Suggested change
private var _image: UIImage {
switch self {
case .chevron: return .chevron
case let .custom(image: image): return image
}
}
var image: UIImage {
_image.withRenderingMode(.alwaysTemplate)
}
var image: UIImage {
switch self {
case .chevron:
return .chevron.withRenderingMode(.alwaysTemplate)
case let .custom(image: image):
return image.withRenderingMode(.alwaysTemplate)
}
}

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 point is that the image should have the same color as the title, so it should be rendered as a template to be tinted. The idea is that I didn't want to set all the image cases with .withRenderingMode(.alwaysTemplate). The private _image is the image itself and the published property image is which sets the property

return imageView
}()

private lazy var rightImageHeightConstraint: NSLayoutConstraint = rightImageView.heightAnchor.constraint(equalToConstant: 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

because the value of the constant is 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is just an arbitrary value for the definition of the constraint

@WanaldinoTelefonica
Copy link
Contributor Author

When the link isLoading, is the chevron hidden?

Yes, AFAIK the chevron only shown on the right of the main title, not on loading

Copy link

@aweell aweell left a comment

Choose a reason for hiding this comment

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

I see the right and the left alignment, in order to anticipate future evolutions, does it make sense to add the vertical bleeding alignment in this PR? When the buttonLink goes into a title, the size of the button container is making the title container to be bigger than it should be, that's why the need of this property. https://jira.tid.es/browse/IOS-9201

Copy link

@aweell aweell left a comment

Choose a reason for hiding this comment

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

When I change the preferences of font size in the device in UIKit (In SwiftUI works fine), the primary, secondary and danger buttons seem to react increasing the font size, but the buttonLink stays always unaltered, is this an already known issue?

@WanaldinoTelefonica
Copy link
Contributor Author

I see the right and the left alignment, in order to anticipate future evolutions, does it make sense to add the vertical bleeding alignment in this PR? When the buttonLink goes into a title, the size of the button container is making the title container to be bigger than it should be, that's why the need of this property. https://jira.tid.es/browse/IOS-9201

I think this should only target adding the chevron for creating the new bottomsheet functionality

When I change the preferences of font size in the device in UIKit (In SwiftUI works fine), the primary, secondary and danger buttons seem to react increasing the font size, but the buttonLink stays always unaltered, is this an already known issue?

Yes, we know but it was deprioritized. If there is time before the release we have this task to add it https://jira.tid.es/browse/IOS-9244

Copy link

@aweell aweell left a comment

Choose a reason for hiding this comment

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

LGTM!

* fix(Button): Fix full width

* fix(Button): Update tests

* fix(Button): Fix center view

* record failing tests of the first brand

* record failing tests of the other brands

---------

Co-authored-by: Alejandro Megías <[email protected]>
@WanaldinoTelefonica WanaldinoTelefonica merged commit 977b1d9 into main Aug 18, 2023
2 checks passed
@WanaldinoTelefonica WanaldinoTelefonica deleted the IOS-9080-Button-chevron branch August 18, 2023 08:30
@tuentisre
Copy link
Collaborator

🎉 This PR is included in version 26.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.

6 participants