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: add progress bar component #3

Merged
merged 17 commits into from Mar 24, 2022
Merged

Conversation

daniel-dumortier
Copy link
Contributor

@daniel-dumortier daniel-dumortier commented Jan 3, 2022

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 :

  • either create non relevant extension
  • or artificially change access control of some properties to be able to put them in other files

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

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.

You can maybe try to reduce VitaminProgressbar by extracting linear or circular code in an other file.
I think that you can also add the final keyword to your classes.

@daniel-dumortier
Copy link
Contributor Author

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
Copy link
Contributor

And for the final keyword to your classes?

@daniel-dumortier
Copy link
Contributor Author

daniel-dumortier commented Jan 6, 2022

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
Copy link
Contributor

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

@daniel-dumortier
Copy link
Contributor Author

daniel-dumortier commented Jan 6, 2022

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

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.

Perfect 👍

@lauthieb lauthieb changed the title feat(progressbar): add the progressbar component feat(progressbar): add component Jan 17, 2022
@lauthieb lauthieb added the enhancement 🚀 New feature or request label Jan 17, 2022
@lauthieb lauthieb changed the title feat(progressbar): add component feat: add progress bar component Jan 21, 2022
@lauthieb
Copy link
Member

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

@CLAassistant
Copy link

CLAassistant commented Feb 14, 2022

CLA assistant check
All committers have signed the CLA.

baptistedajon
baptistedajon previously approved these changes Feb 14, 2022
Copy link
Contributor

@baptistedajon baptistedajon left a comment

Choose a reason for hiding this comment

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

🍫

@daniel-dumortier daniel-dumortier added the Ready for Design Review The showcase is ready for design review label Mar 1, 2022
# Conflicts:
#	Showcase/Vitamin Showcase.xcodeproj/project.pbxproj
@baptistedajon baptistedajon self-requested a review March 17, 2022 12:54
baptistedajon
baptistedajon previously approved these changes Mar 17, 2022
Copy link
Contributor

@baptistedajon baptistedajon left a comment

Choose a reason for hiding this comment

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

🍆

@daniel-dumortier daniel-dumortier removed the Ready for Design Review The showcase is ready for design review label Mar 17, 2022
FannyDemey and others added 3 commits March 17, 2022 16:06
* fix: proposal for accessibility of progress bar

* refacto - improve accessibility Label setting
@sonarcloud
Copy link

sonarcloud bot commented Mar 18, 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 added the Ready for Design Review The showcase is ready for design review label Mar 18, 2022
@daniel-dumortier daniel-dumortier merged commit 88538e8 into main Mar 24, 2022
@daniel-dumortier daniel-dumortier deleted the component/progressbar branch March 24, 2022 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🚀 New feature or request Ready for Design Review The showcase is ready for design review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add progress bar component
6 participants