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

Elvis/status button #270

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Elvis/status button #270

wants to merge 2 commits into from

Conversation

emarc0314
Copy link
Contributor

Overview

Added the status button + cute animations to the home screen.

Next Steps (delete if not applicable)

Implementing button action.

Screen Recording

Button
button.mp4

Copy link
Contributor

@tiffany-pann tiffany-pann left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -62,7 +62,8 @@ class HomeViewController: UIViewController {

override func viewWillAppear(_ animated: Bool) {
super.viewWillAppear(animated)
navigationController?.tabBarController?.tabBar.isHidden = false
// navigationController?.tabBarController?.tabBar.isHidden = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double checking, this is a temporary change right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup!

make.width.equalTo(60)
make.height.equalTo(40)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Insert line after this }

Copy link
Contributor

@aldenlamp aldenlamp left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few small changes here and there
Some notes:

  • You should turn on the swiftlint
  • I would appreciate a PR title of a oneline description instead of j the branch name
  • You should change the 'to' branch to be dev/homescreencapacity or some intermediate like that just to keep main fully functional at all times :))

Copy link
Contributor

Choose a reason for hiding this comment

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

Double-check to see if this is not already in the assets folder. There is an asset titled "down-arrow-solid." Is this different than that?

@@ -9,17 +9,18 @@ import SnapKit
import UIKit

class HomeScreenHeaderView: UIView {

Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace; you should turn on the swiftlint and make sure there's no styling related warnings before PR.

var didSetupShadow = false

Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace

override init(frame: CGRect) {
super.init(frame: frame)

Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace

// BACKGROUND COLOR
backgroundColor = .white

Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace

Comment on lines +115 to +125
make.width.equalTo(24)
make.height.equalTo(24)
make.centerY.equalToSuperview()
make.leading.equalToSuperview().offset(10)
}

dropDownImageView.snp.makeConstraints { make in
make.width.equalTo(8)
make.height.equalTo(8)
make.centerY.equalTo(circularView)
make.trailing.equalToSuperview().inset(10)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it could be good to separate out all the constants in an enum LayoutConstants at the top of the class.

Comment on lines +30 to +33
private func createCircularPath() {
self.layer.cornerRadius = width!/2
let circularPath = UIBezierPath(arcCenter: CGPoint(x: width/2, y: width/2) , radius: (width - 1.5)/2, startAngle: CGFloat(-0.5 * .pi), endAngle: CGFloat(2.0 * .pi), clockwise: true)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to be used for the capacities? If so, I think it would be cool to make this more generic maybe with a percent, start angle, andlinewidth (and maybe animated??). Ofc that's something that could be done later as well.


class StatusButton: UIButton {

var circularView: UIView!
Copy link
Contributor

Choose a reason for hiding this comment

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

of type CircleProgressView instead of UIView


@objc func showCapacityView() {
if (statusButton.tag == 0) {
statusButton.backgroundColor = .gray00
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the design has it as white? imo based on the video having that extra contrast of white feels like it would make it more noticeable.



@objc func showCapacityView() {
if (statusButton.tag == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it would enhance readability to use a boolean variable instead of the tag property. Maybe as a property of statusButton like statusButton.isDisclosed.

Copy link
Contributor

Choose a reason for hiding this comment

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

To add on, you could also do this in a way which moves a lot of functionality into the status button. This function could literally have two lines:
statusButton.toggleDisclosure()
delegate.didToggleDisclosure() // For communicating with the homeviewcontroller
and then everything else such as background color could stay in the status button

Copy link

@antoinztte antoinztte left a comment

Choose a reason for hiding this comment

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

Echoing Tiff and Alden's comments!

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.

4 participants