Skip to content

Conversation

@RedenticDev
Copy link

Let me know what you think, probably gonna upload some more cells later as this repo is very useful.

* Add an alternative to the Animated Title View with
support for an icon
* Add a new SF Symbols PSLinkCell to use Apple's
native symbols into PSLinkCell
* Modify README to use relative links
- Added animated banner with a bunch of features
- Improved SF Symbols cell by simplifying and improving processes
- Fixed a small issue on Animated Title View With Icon
@LacertosusRepo
Copy link
Owner

I really like the addition of the icon to the AnimatedTitleView, but it should be a part of the same class not in a separate file. I've attached source files that combines the classes.

@RedenticDev
Copy link
Author

Working on it. Side note: why do you use categories instead of declaring properties or ivars?

@RedenticDev
Copy link
Author

This should work, I let you checking out. I didn't test that. Just remove the "second part" of the .h file to fix the merge conflict.

@LacertosusRepo
Copy link
Owner

Side note: why do you use categories instead of declaring properties or ivars?

What do you mean?

@RedenticDev
Copy link
Author

Side note: why do you use categories instead of declaring properties or ivars?

What do you mean?

You're using curly braces in @implementation and I thought it was a class extension, my bad. But what are the upsides of doing so on some codes I've seen? People were using @interface in their .h, extension with their variables in the .m, and @implementation in .m too

@LacertosusRepo
Copy link
Owner

Declaring ivars in the implementation just defaults them to @private vs the interface being @public. It's just preference to me, I try to only use properties if something need access to a value/object.

@RedenticDev
Copy link
Author

What's the point of using ivars then?

@LacertosusRepo LacertosusRepo self-assigned this Oct 12, 2021
@LacertosusRepo
Copy link
Owner

What's the point of using ivars then?

Because nothing needs access to the variables. They are just for this class so there's no point in making properties.

@RedenticDev
Copy link
Author

Okay I see thanks. I let you check the PR.

stackView.translatesAutoresizingMaskIntoConstraints = NO;
[self addSubview:stackView];

[NSLayoutConstraint activateConstraints:@[
Copy link
Owner

@LacertosusRepo LacertosusRepo Oct 19, 2021

Choose a reason for hiding this comment

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

Some ideas here about the constraints:

The stackview wont be centered on the X axis with the current constraints. All edges should be anchored or it could be centered with both centerX/centerY anchors.

Using edge anchors would also allow you to get rid of the heightAnchor by constraining the top anchor of the stackview to the title label's gradient's top anchor and the bottom anchor to the subtitle's bottom anchor:

[self.stackView.topAnchor constraintEqualToAnchor:self.gradientView.topAnchor],
[self.stackView.bottomAnchor constraintEqualToAnchor:self.titleLabel.bottomAnchor],

The gradient's height/width anchors should just be equal to the title label's anchors: I see that the label is used as the mask.

EDIT: Fixed some incorrect suggestions.

Copy link
Author

Choose a reason for hiding this comment

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

For the first part, the stack view isn't aimed to be centered on X, its left padding with a small indent of 25 (to leave some space and not stick it to the left border) looks great. In my opinion if the end user/developer wants it centered it's easily doable, but it's not the aim of my implementation.

For your edge anchors suggestion, it should work too, but I'm not sure if it'd break the gradient or the scrolling effect I made. Also I may add that it works great as-is, thus I don't get the point of changing that.

If you're really sure that it won't break the current look & feel of the banner, and that a change might be better in terms of performance or code practices, I'm open to any change. But honestly I don't see why anything should be changed on constraints' side with the current implementation.

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.

2 participants