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

Posts and Pages search: Update to Muriel color scheme #12124

Merged
merged 10 commits into from
Jul 17, 2019
Merged

Posts and Pages search: Update to Muriel color scheme #12124

merged 10 commits into from
Jul 17, 2019

Conversation

leandroalonso
Copy link
Contributor

@leandroalonso leandroalonso commented Jul 15, 2019

This PR fixes the color and some design details in posts and pages search.

To test:

Before After

@nheagy nheagy requested a review from frosty July 15, 2019 23:05
@nheagy nheagy self-assigned this Jul 15, 2019
@nheagy nheagy added the Muriel label Jul 15, 2019
@nheagy nheagy added this to the 13.0 milestone Jul 15, 2019
Copy link
Contributor

@frosty frosty left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Changes look good to me, just two small comments in the code :)

updateBackgroundColor()
}

// Update controller's background color to avoid white lines
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add just a little more clarification to this comment? Where was the white line and why was it appearing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed! Let me know what you think.

@@ -83,6 +86,12 @@ class PostCompactCell: UITableViewCell, ConfigurablePostView {
contentView.backgroundColor = innerView.backgroundColor
}

private func setupSeparator() {
WPStyleGuide.applyBorderStyle(separator)
contentView.heightAnchor.constraint(equalToConstant: contentView.frame.height + WPStyleGuide.separatorHeight).isActive = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this should be necessary, if you ensure the InnerView is pinned to the top of the content view, and the bottom of the InnerView is pinned to the top of the separator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was adding the separator height to the cell height and realign the labels (to maintain the same margins as before the hardcoded separator). But yeah, complex and not so clear.

I removed it and managed to achieve the same result with a heigh constant in the xib.

Copy link
Contributor

@frosty frosty left a comment

Choose a reason for hiding this comment

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

Looks great!

@frosty frosty merged commit c0bd679 into wordpress-mobile:develop Jul 17, 2019
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