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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add factory for AppInstallPage #12581

Merged
merged 5 commits into from
Jul 9, 2024

Conversation

AdalbertoMoz
Copy link
Collaborator

@AdalbertoMoz AdalbertoMoz commented Jul 4, 2024

Description

The code below introduces a Factory for the AppInstallPage, serving as a replacement for the YoutubeRegretsReporterExtensionPage, which was removed in a previous pull request -> #12573

The following fields were populated with fake data:

  • title
  • slug
  • hero_heading
  • hero_subheading
  • hero_background
  • download_buttons
  • cta
  • body

For further details, please refer to the code.

Test

This the link that you can visit to review the AppInstallPage that was created with the Factory

image

Link to sample test page: https://foundation-s-feature-12-cislwu.herokuapp.com/en/youtube/regretsreporter/
Related PRs/issues: #12218

Checklist

Tests

  • Is the code I'm adding covered by tests?

Changes in Models:

  • Did I update or add new fake data?
  • Did I squash my migration?
  • Are my changes backward-compatible. If not, did I schedule a deploy with the rest of the team?

Documentation:

  • Is my code documented?
  • Did I update the READMEs or wagtail documentation?

Merge Method
馃挕鉂桼emember to use squash merge when merging non-feature branches into main

鈹咺ssue is synchronized with this Jira Story

@AdalbertoMoz AdalbertoMoz self-assigned this Jul 4, 2024
@AdalbertoMoz AdalbertoMoz temporarily deployed to foundation-s-feature-12-3ajkaa July 4, 2024 15:01 Inactive
Copy link
Collaborator

@danielfmiranda danielfmiranda left a comment

Choose a reason for hiding this comment

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

Hi @AdalbertoMoz, thanks so much for taking on this work! 馃檶

I gave the code a look and ran inv new-db, and can confirm that everything is working perfectly.

I just have 3 small pieces of feedback that I think will wrap up this factory nicely.

If you need any help or have questions, feel free to reach out. I'd be happy to work through it together 馃槃

@AdalbertoMoz AdalbertoMoz temporarily deployed to foundation-s-feature-12-3ajkaa July 9, 2024 01:52 Inactive
@AdalbertoMoz AdalbertoMoz temporarily deployed to foundation-s-feature-12-cislwu July 9, 2024 02:06 Inactive
@AdalbertoMoz
Copy link
Collaborator Author

AdalbertoMoz commented Jul 9, 2024

Hey @danielfmiranda, thank you so much for your detailed PR review! Your feedback is always incredibly valuable.

I've implemented your suggested changes in this branch and it's now ready for another review. 馃殌

Note: I just changed the PR description to include the new Review App link where you can find the latest version of the AppInstallPageFactory

@AdalbertoMoz AdalbertoMoz temporarily deployed to foundation-s-feature-12-cislwu July 9, 2024 17:42 Inactive
@AdalbertoMoz AdalbertoMoz temporarily deployed to foundation-s-feature-12-cislwu July 9, 2024 17:47 Inactive
Copy link
Collaborator

@danielfmiranda danielfmiranda 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 the updates @AdalbertoMoz 馃檶! I just tested out the latest changes and can confirm everything is working great. LGTM! 馃殌

@AdalbertoMoz AdalbertoMoz temporarily deployed to foundation-s-feature-12-unf2j0 July 9, 2024 18:10 Inactive
@AdalbertoMoz AdalbertoMoz merged commit a9fe180 into main Jul 9, 2024
6 checks passed
@AdalbertoMoz AdalbertoMoz deleted the feature/12218-add-factories-for-AppInstallPage branch July 9, 2024 18:18
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.

None yet

2 participants