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

Hiding dashboard cards: add context menus #20384

Merged
merged 10 commits into from
Apr 3, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ class BlogDashboardCardFrameView: UIView {
addSubview(buttonContainerStackView)

NSLayoutConstraint.activate([
buttonContainerStackView.topAnchor.constraint(equalTo: topAnchor, constant: Constants.buttonContainerStackViewPadding),
buttonContainerStackView.topAnchor.constraint(equalTo: topAnchor),
buttonContainerStackView.trailingAnchor.constraint(equalTo: trailingAnchor, constant: -Constants.buttonContainerStackViewPadding)
])

Expand Down Expand Up @@ -251,11 +251,11 @@ class BlogDashboardCardFrameView: UIView {
private enum Constants {
static let bottomPadding: CGFloat = 8
static let headerPaddingWithEllipsisButtonHidden = UIEdgeInsets(top: 12, left: 16, bottom: 8, right: 16)
static let headerPaddingWithEllipsisButtonShown = UIEdgeInsets(top: 12, left: 16, bottom: 8, right: 8)
static let headerPaddingWithEllipsisButtonShown = UIEdgeInsets(top: 4, left: 16, bottom: 0, right: 8)
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested the changes on the latest trunk as I was interested in how these changes affect the latest development.

The top of 4 for a multiline label looks visually too close. It looks like a title label centers vertically together with the button. When we have a single-line label, the top of 4 looks fine, and 8 is too far.

image

Copy link
Contributor Author

@kean kean Mar 31, 2023

Choose a reason for hiding this comment

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

Thanks, good catch. I tested only with the existing cards, which are single-line. titleLabel.numberOfLines = 0 was just added to trunk this week.

I reverted this change for now. It's a production behavior, and I don't want this change to hold the main PRs. I'll think of a different way to implement it. I could do it using hitTest, then there won't be any layout changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, retested it, looks okay now, although yes, now the button appears a bit too low, so this part as you mentioned could be improved. 👍

static let headerHorizontalSpacing: CGFloat = 5
static let iconSize = CGSize(width: 18, height: 18)
static let cornerRadius: CGFloat = 10
static let ellipsisButtonPadding = UIEdgeInsets(top: 0, left: 8, bottom: 0, right: 8)
static let ellipsisButtonPadding = UIEdgeInsets(top: 8, left: 8, bottom: 8, right: 8)
static let buttonContainerStackViewPadding: CGFloat = 8
static let mainStackViewTrailingPadding: CGFloat = 32
}
Expand Down