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

feat(button): VitaminButton swiftUI implementation #76

Merged
merged 15 commits into from Sep 9, 2022

Conversation

daniel-dumortier
Copy link
Contributor

@daniel-dumortier daniel-dumortier commented Jul 26, 2022

Changes description

this PR aims to provide a Swift UI implementation for VitaminButton.
It is still in draft, because some points are to be finalized or discussed

Context

related issue still to be created

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch. Please, don't request directly from your main!
  • Check commits & PR names matches our requested structure. It must follow the https://www.conventionalcommits.org pattern.
  • Check your code additions will fail neither code linting checks.
  • I have reviewed the submitted code.
  • I have tested on an iPhone device/simulator.
  • I have tested on an iPad device/simulator.
  • If it includes design changes, please ask for a review with a core team designer.

Does this introduce a breaking change?

  • No

Screenshots

I will add screenshots when finalized

iPhone

iPad

Other information

For now, the base branch is the UIKit VitaminButton icon alone branch, will change to develop once validated and merged

@daniel-dumortier daniel-dumortier marked this pull request as draft July 26, 2022 11:56
Base automatically changed from feat/button_icon_alone to main July 26, 2022 15:30
@daniel-dumortier
Copy link
Contributor Author

I decided to commit an intermediate solution for the problem of reusing VitaminButtonIconType :

  • I created an specific IconType enum for swiftUI, that can still have an UIImage associated value, but a Image.TemplateRenderingMode (and not a UIImage.RenderingMode anymore)
  • this IconType has an internal computed property of type VitaminButtonIconType

With this approach, I think we combine two advantages :

  • the associated values of IconType are more "swiftUI" (even if it still uses UIImage, but after all, Vitamix also deals with UIImage)
  • all the logic and computed properties of VitaminButtonIconType can be reused for swiftUI

Any feedbacks on this approach (and if it is better than the previous) are welcome ;)

Copy link
Contributor

@florentlotthepro florentlotthepro left a comment

Choose a reason for hiding this comment

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

Nice work 👍
Just a few comments.

Sources/VitaminUIKit/Components/Button/VitaminButton.swift Outdated Show resolved Hide resolved
Sources/VitaminUIKit/Components/Button/VitaminButton.swift Outdated Show resolved Hide resolved
@daniel-dumortier daniel-dumortier marked this pull request as ready for review August 5, 2022 17:19
baptistedajon
baptistedajon previously approved these changes Aug 22, 2022
Sources/VitaminSwiftUI/Components/Button/README.md Outdated Show resolved Hide resolved
Sources/VitaminSwiftUI/Components/Button/README.md Outdated Show resolved Hide resolved
Sources/VitaminSwiftUI/Components/Button/README.md Outdated Show resolved Hide resolved
Sources/VitaminSwiftUI/Components/Button/README.md Outdated Show resolved Hide resolved
Sources/VitaminUIKit/Components/Button/VitaminButton.swift Outdated Show resolved Hide resolved
Co-authored-by: Florent LOTTHÉ <florent.lotthe@decathlon.com>
@sonarcloud
Copy link

sonarcloud bot commented Sep 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@daniel-dumortier daniel-dumortier merged commit 8c18aee into main Sep 9, 2022
@daniel-dumortier daniel-dumortier deleted the feat/swiftui_vitamin_button branch September 9, 2022 14:44
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.

None yet

3 participants