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

Feature/active label #55

Merged
merged 23 commits into from
Jul 8, 2019
Merged

Feature/active label #55

merged 23 commits into from
Jul 8, 2019

Conversation

JobsIsMyHomeboy
Copy link
Contributor

This adds a new subclass of UILabel called ActiveLabel. It can be used to show activity for a label while maintaining some spacing on the screen as well as visual indicator that something is happening. The use of nil on the label is what turns the activity indicator on and off.

- Modified podspec for ActiveLabel subspec.
- Increased version to 1.3.6
- Updated readme with details for ActiveLabel.
- Added ActiveLabel to Examples
- Added Unit Tests for ActiveLabel.
@JobsIsMyHomeboy JobsIsMyHomeboy marked this pull request as ready for review April 16, 2019 13:33
Copy link
Contributor

@tylermilner tylermilner left a comment

Choose a reason for hiding this comment

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

One question I have is how does this behave when using dynamic type? Do the gradient animations scale accordingly?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Tests/ActiveLabelTests.swift Show resolved Hide resolved
UtiliKit.podspec Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Tests/ActiveLabelTests.swift Show resolved Hide resolved
JobsIsMyHomeboy and others added 9 commits April 17, 2019 06:50
- Added some backticks to the ActiveLabel documentation.
- Added some backticks to the documentation.
- Created loadingGray static class constant to be used for the default color.
- Removed use of defer in initializers since it isn't needed.
- Moved constraint setup into its own function which is called when configuring the loading views.
- Cleaned up code.
- Modified UnitTests and Added specific Snapshot Tests.
Replaced isLoading Bool to now use the State enum.
Tweaked documentation.
…g indicators do not scale with Dynamic Type.
@JobsIsMyHomeboy
Copy link
Contributor Author

@tylermilner Due to some weirdness with trying to dynamically size the loading views based on the Content Size Category change I don't feel like supporting it at this time is worthwhile. I have some ideas on how to handle it, but I don't think they would be worth introducing in a version 1 of this class. I've added a Note to the class documentation calling out that the loading views don't support resizing via dynamic type.

Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
…ced under ActiveLabel.

Changed ActiveLabel configuration properties to used the Configuration.default values.
Removed showLoadingViewsInStoryboard and isSnapshotTesting in favor of a private isGradientCentered bool to signal centering the gradient.
Added public function configureForSnapshotTesting that sets isGradientCentered to true in order to keep that variable private.
Moved configurationChanged() to private and added didSet to all configuration properties that now call configurationChanged()
Updated default state value to .text(nil)
Added configuredForSnapshotTest public function in order to center the gradient for snapshot tests.
Fixed textDidUpdate to use new State initializer.
Corrected some comments.
Fixed tests to work properly with latest changes.
Updated ReadMe to reflect changes.
Added line to ReadMe about configuring for snapshot tests.
@JobsIsMyHomeboy
Copy link
Contributor Author

I just pushed an update that addresses the open issues for the PR.

Copy link
Contributor

@tylermilner tylermilner left a comment

Choose a reason for hiding this comment

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

@JobsIsMyHomeboy I think this should be good to go after you add a CHANGELOG entry!

Sources/UtiliKit/ActiveLabel/ActiveLabel.swift Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Jun 18, 2019

Codecov Report

Merging #55 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #55    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files           3      4     +1     
  Lines         204    413   +209     
======================================
+ Hits          204    413   +209
Impacted Files Coverage Δ
Tests/ActiveLabelTests.swift 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d536e3f...fe9cc31. Read the comment docs.

tylermilner
tylermilner previously approved these changes Jun 18, 2019
@tylermilner
Copy link
Contributor

@JobsIsMyHomeboy It looks like Codebeat is complaining about a few issues regarding ActiveLabel. Can you take a look and see what you can easily fix?

wmcginty
wmcginty previously approved these changes Jun 21, 2019
@JobsIsMyHomeboy
Copy link
Contributor Author

@wmcginty @tylermilner I don't have write access so I can't merge. :(

@wmcginty
Copy link
Contributor

Is there anything we can do to fix any/some of those Codebeat issues @JobsIsMyHomeboy ?

…onfiguration by adding a secondary struct for the LoadingView portion of the variables.

Updated ReadMe and fixed tests with configuration changes.
@JobsIsMyHomeboy JobsIsMyHomeboy dismissed stale reviews from wmcginty and tylermilner via fe9cc31 July 8, 2019 14:09
@tylermilner tylermilner merged commit 4011dc7 into BottleRocketStudios:master Jul 8, 2019
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

4 participants