Automattic / Gravatar-SDK-iOS

Gravatar SDK is a Swift library that allows you to integrate Gravatar features into your own iOS applications.
https://gravatar.com
Mozilla Public License 2.0
36 stars 1 forks source link

Adding profile view empty state #237

Closed etoledom closed 4 months ago

etoledom commented 4 months ago

Closes #234

Description

This PR adds an empty state to the profile cards.

Testing Steps

etoledom commented 4 months ago

Thanks for the review, I'll check out the comments.

About this one:

  1. I wasn't able to see the dark mode of the empty states in the demo app. They look light even if I select dark.

That's odd, it works well for me:

CleanShot 2024-05-02 at 14 00 41

etoledom commented 4 months ago

Updated with review comments.

In figma there are some dashed lines around the "Tell the world who you are..." section.

I totally missed this one.

I spent some time tinkering about this one. The dash lines plus the padding makes me think that a good path to go is either UIView wrapper of the label or using a UITextView.

As our public API is a UILabel, and the builders work based on UILabel, ideally we keep UILabel as the public interface, and we can keep using it with the AboutMeBuilder, so I tried to keep it as is as much as possible, but this forced the usage of a UILabel subclass.

Some extra thoughts:

With the UIView wrapper, it's simple to add the dashes and also the padding with constraints. Drawbacks:

Then I thought about a protocol composition, but some overrides are needed anyway, so though it would help to share this code and make a DashedView, DashedButton, etc... as for now we don't need them, and the refactor is simple, and it won't help overcoming the drawbacks of the UILabel subclass.

Finally, the UILabel subclass. I like that we can keep the public api as UILabel, the AboutMeBuilder keeps working without modifications and the BaseProfileView subclasses don't need modifications either. Drawbacks:

I try to avoid those overrides, but in this case it feels better than the UIView wrapper option.

Still, I'm happy to reconsider.

pinarol commented 4 months ago

I like that we can keep the public api as UILabel, the AboutMeBuilder keeps working without modifications and the BaseProfileView subclasses don't need modifications either.

Exactly. Even though the internal implementation gets a bit more complex, subclassing makes sense here. I thought similar things.

etoledom commented 4 months ago

Thank you for the review 🙏