Decathlon / vitamin-ios

Decathlon Design System UI components for iOS & iPadOS applications
https://www.decathlon.design
Apache License 2.0
46 stars 14 forks source link

feat: add progress bar component #3

Closed daniel-dumortier closed 2 years ago

daniel-dumortier commented 2 years ago

Changes Description

Here is a proposal of implementation for progress bars. It handles the two variants (linear, circular), each sub-style (empty, image, percentage), size (small, medium, large) of them. It also handles the progress type (determinate and indeterminate)

The Showcase has been updated to demonstrate all possible configurations for progressbar.

The documentation has been added, and the general Readme has been updated to reference this documentation.

Remarks

1 - there is still a SwiftLint warning regarding the file length. I decided not to fix it because it would require to :

2 - The content of this PR has already partially been reviewed by @florentlotthepro on the inner-source repo, I just refactored the showcase and doc parts, fixed @florentlotthepro feedbacks, and fixed some problems with dark mode

daniel-dumortier commented 2 years ago

I tried, but most of the code is organized with common functions for both variant, and a test on the variant in the body of the method. Splitting it in two would require to break all the code, which IMHO is not worth just to reduce the file that is less than 100 lines above the suggested Swiftlint limit.

And when I try to move extensions in other files, I always struggle with private properties/functions, that are not visible. And I am not sure this is a good idea to reduce the access control just to match the Swiftlint rule about file size.

I was just able to extract colors in a separate file. And I extracted the background and label colors in this extension.

florentlotthepro commented 2 years ago

And for the final keyword to your classes?

daniel-dumortier commented 2 years ago

I added the final class on the VitamoinProgressbartclass.

Did you mean to put it on Showcase classes ? If yes, I can do that, but I am not sure to get the point since they are classes of the Showcase, they won't be packaged and distributed, and thus, I am not really sure to understand who we would like to avoid subclassing these classes... ;)

florentlotthepro commented 2 years ago

With a lot of classes, it could speed up the build, it's principally for that.

daniel-dumortier commented 2 years ago

Honesttly, I did not know that. So let's go for it ! And sorry for reinvalidating your review with this commit ;)

lauthieb commented 2 years ago

@florentlotthepro @baptistedajon is it ok to merge it soon? :)

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

sonarcloud[bot] commented 2 years ago

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