Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

@jamshaidazeem
Copy link

No description provided.

Copy link
Contributor

@victorBaro victorBaro left a comment

Choose a reason for hiding this comment

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

Thank you for submitting a PR.
Please take a look at my comment. I would be happy to merge once it is fixed.

}

fileprivate let lineView = AnimatedLine()
public let lineView = AnimatedLine()
Copy link
Contributor

Choose a reason for hiding this comment

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

Although this works, I would prefer having 2 new methods than making the line public. Something like:
func hideLine(animated: Bool)
Internally this would either call isHidden or fillLine(with: UIColor.clear).

func showLine(animated: Bool)
Internally this would either call isHidden = false or fillLine(with: style.lineColor)

Copy link
Author

Choose a reason for hiding this comment

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

thanks

}

fileprivate let lineView = AnimatedLine()
public let lineView = AnimatedLine()

Choose a reason for hiding this comment

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

can't be this solve just by making an open var?? e.g.

    open var lineViewHidden: Bool = false {
        didSet {
            if lineViewHidden {
                lineView.fillLine(with: UIColor.clear)
            } else {
                lineView.fillLine(with: style.lineActiveColor)
            }
        }
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that will work as well.
My concern about making lineView public was that we would be exposing implementation details that should be private.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants