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

fix: display underwriters with default placement value #2061

Merged
merged 2 commits into from
Mar 17, 2023

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Mar 15, 2023

All Submissions:

Changes proposed in this Pull Request:

Fixes an issue with default sponsor meta values. The default values for several meta fields is inherit, but the theme expects them to be something else, which causes some issues with display when defaults are used. This mainly affects the display of underwriter-type sponsors when no post-level overrides are set.

Closes 1200550061930446/1204158091646412

How to test the changes in this Pull Request:

  1. On master, publish a new sponsor with a title and some content and set the scope to Underwritten Content. Leave everything else default/blank.
  2. Apply the underwriter to a post and leave all Sponsor Display Overrides to their defaults ("Inherit from sponsor").
  3. On the front-end observe that the underwriter info isn't displayed at all.
  4. Check out this branch, confirm that now the underwriter info is displayed at the top of the post (the default placement).
  5. Play with different sponsor settings in the sponsor and the corresponding override settings in the post, and ensure that the display settings are reflected on the front-end (sponsor settings are used unless a different override is set on the post).

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@dkoo dkoo added [Type] Bug Incorrect behavior or functionality [Status] Needs Review The issue or pull request needs to be reviewed labels Mar 15, 2023
@dkoo dkoo requested a review from laurelfulford March 15, 2023 22:15
@dkoo dkoo requested a review from a team as a code owner March 15, 2023 22:15
@dkoo dkoo self-assigned this Mar 15, 2023
Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

This looks good to me, @dkoo! 🙌

Confirmed that the updates fixed the issue, and also that the sponsor settings are still being applied properly (changing the settings on the sponsor level, and on the per-post level, both work!).

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Mar 17, 2023
@dkoo dkoo merged commit 265c049 into master Mar 17, 2023
@dkoo dkoo deleted the fix/sponsor-default-underwriter-placement branch March 17, 2023 19:32
matticbot pushed a commit that referenced this pull request Mar 31, 2023
## [1.69.4-alpha.1](v1.69.3...v1.69.4-alpha.1) (2023-03-31)

### Bug Fixes

* display underwriters with default placement value ([#2061](#2061)) ([265c049](265c049))
* make sure wide blocks don't overflow containers ([#2063](#2063)) ([5fea5cf](5fea5cf))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.69.4-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Apr 10, 2023
## [1.69.4](v1.69.3...v1.69.4) (2023-04-10)

### Bug Fixes

* display underwriters with default placement value ([#2061](#2061)) ([265c049](265c049))
* make sure wide blocks don't overflow containers ([#2063](#2063)) ([5fea5cf](5fea5cf))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.69.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @alpha released [Status] Approved The pull request has been reviewed and is ready to merge [Type] Bug Incorrect behavior or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants