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

[Fairground] Deprecate and rename fields for new card attributes #106

Conversation

waisingyiu
Copy link
Contributor

@waisingyiu waisingyiu commented Sep 12, 2024

What does this change?

New types of cards have been introduced as part of the homepage redesign work, and the Card schema has been updated to enable MAPI to dictate which card UI to render via the collection response.

The previous PR #100 added new card size and boost level attributes. This pull request makes other changes to the schema as proposed:

  • deprecate boosted, compact, mega_boosted, tail_image_size, trail_image_aspect_ratio, boosted_headline and headline_position fields in Card
  • rename sublinks_arrangement to preferred_sublinks_arrangement by deprecating the old one and creating a new one

Although we want to deprecate the boosted and compact attributes and the CARD_TYPE_HIGHLIGHT card type, we cannot make them as deprecated in the proto file because production apps are consuming these attributes and we must continue to populate these attributes. Setting values in a deprecated field results in a warning that, under the current MAPI compilation settings, fails the whole compilation.

@waisingyiu waisingyiu requested a review from a team as a code owner September 12, 2024 14:53
@gu-scala-library-release
Copy link
Contributor

@waisingyiu has published a preview version of this PR with release workflow run #62, based on commit 3878e5e:

1.0.24-PREVIEW.fairgrounddeprecate-and-rename-fields-for-new-card-attributes.2024-09-12T1455.3878e5ec

Want to make another preview release?

Click 'Run workflow' in the GitHub UI, specifying the fairground/deprecate-and-rename-fields-for-new-card-attributes branch, or use the GitHub CLI command:

gh workflow run release.yml --ref fairground/deprecate-and-rename-fields-for-new-card-attributes

Want to make a full release after this PR is merged?

Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command:

gh workflow run release.yml

@waisingyiu waisingyiu force-pushed the fairground/deprecate-and-rename-fields-for-new-card-attributes branch from 3878e5e to 31f36e3 Compare September 12, 2024 15:07
@gu-scala-library-release
Copy link
Contributor

@waisingyiu has published a preview version of this PR with release workflow run #63, based on commit 31f36e3:

1.0.24-PREVIEW.fairgrounddeprecate-and-rename-fields-for-new-card-attributes.2024-09-12T1508.31f36e36

Want to make another preview release?

Click 'Run workflow' in the GitHub UI, specifying the fairground/deprecate-and-rename-fields-for-new-card-attributes branch, or use the GitHub CLI command:

gh workflow run release.yml --ref fairground/deprecate-and-rename-fields-for-new-card-attributes

Want to make a full release after this PR is merged?

Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command:

gh workflow run release.yml

@waisingyiu waisingyiu force-pushed the fairground/deprecate-and-rename-fields-for-new-card-attributes branch from 31f36e3 to 07a5400 Compare September 12, 2024 15:31
@waisingyiu waisingyiu force-pushed the fairground/deprecate-and-rename-fields-for-new-card-attributes branch from 07a5400 to 963d18b Compare September 12, 2024 15:32
@gu-scala-library-release
Copy link
Contributor

@waisingyiu has published a preview version of this PR with release workflow run #64, based on commit 963d18b:

1.0.24-PREVIEW.fairgrounddeprecate-and-rename-fields-for-new-card-attributes.2024-09-12T1533.963d18ba

Want to make another preview release?

Click 'Run workflow' in the GitHub UI, specifying the fairground/deprecate-and-rename-fields-for-new-card-attributes branch, or use the GitHub CLI command:

gh workflow run release.yml --ref fairground/deprecate-and-rename-fields-for-new-card-attributes

Want to make a full release after this PR is merged?

Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command:

gh workflow run release.yml

Copy link
Contributor Author

@waisingyiu waisingyiu left a comment

Choose a reason for hiding this comment

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

Hey @ab-gnm sorry, I made some more changes after you approved the PR.

I found that accessing deprecated fields in MAPI results in a warning that would fail the compilation (due to our current "warning-as-error" compilation flag). So I should not mark some fields as deprecated if we still need to access them from MAPI, namely the compact, boosted and CARD_TYPE_HIGHLIGHT.

Great if you could take a look at it.

@ab-gnm
Copy link
Member

ab-gnm commented Sep 12, 2024

So I should not mark some fields as deprecated if we still need to access them from MAPI, namely the compact, boosted and CARD_TYPE_HIGHLIGHT.

Do we still need to access these fields in MAPI when we're migrating to the new ones?

@waisingyiu
Copy link
Contributor Author

So I should not mark some fields as deprecated if we still need to access them from MAPI, namely the compact, boosted and CARD_TYPE_HIGHLIGHT.

Do we still need to access these fields in MAPI when we're migrating to the new ones?

I think we still need to populate these fields because existing production apps are looking at compact and boosted attributes to decide on the type of cards for rendering. We still keep the CARD_TYPE_HIGHLIGHT card type for highlights collection because I assume the apps we are using for AB testing are looking at this card type. Or are there anything I miss?

@waisingyiu waisingyiu merged commit 6edd08f into main Sep 13, 2024
9 checks passed
@waisingyiu waisingyiu deleted the fairground/deprecate-and-rename-fields-for-new-card-attributes branch September 13, 2024 15:17
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