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

updated press page content #11766

Merged
merged 7 commits into from
Feb 1, 2024
Merged

Conversation

danielfmiranda
Copy link
Collaborator

@danielfmiranda danielfmiranda commented Jan 25, 2024

Description

Link to sample test page: https://foundation-s-feature-11-nobyi2.herokuapp.com/en/privacynotincluded/about/press/
Related PRs/issues: #11138

This PR updates the PNI press page with the updated copy found here.

@danielfmiranda danielfmiranda temporarily deployed to foundation-s-feature-11-nobyi2 January 25, 2024 01:59 Inactive
@danielfmiranda danielfmiranda temporarily deployed to foundation-s-feature-11-nobyi2 January 25, 2024 02:17 Inactive
@danielfmiranda danielfmiranda temporarily deployed to foundation-s-feature-11-nobyi2 January 25, 2024 02:24 Inactive
@Zoe-PNI
Copy link

Zoe-PNI commented Jan 25, 2024

thank you, Daniel!

Screenshot 2024-01-25 at 10 05 41 AM Screenshot 2024-01-25 at 10 06 19 AM

@mtdenton
Copy link
Contributor

Hi @Zoe-PNI ! Is the copy document final after the changes you requested above? If this is something that Daniel missed, please disregard.

@danielfmiranda let's hold on further commits until we have confirmation from Zoe that this is final state of the copy doc.

Thanks both!

@Zoe-PNI
Copy link

Zoe-PNI commented Jan 26, 2024

@mtdenton yes! that's correct. thank you! cc: @danielfmiranda

@danielfmiranda danielfmiranda temporarily deployed to foundation-s-feature-11-nobyi2 January 26, 2024 18:33 Inactive
@danielfmiranda
Copy link
Collaborator Author

Thanks @mtdenton @Zoe-PNI!

@Zoe-PNI, I have removed "Recognition" from the copy, and also updated the second quote to link to the provided NPR article as requested.

Can you please take another look whenever you find a free moment and let me know if everything looks OK?

Thanks again!

@Zoe-PNI
Copy link

Zoe-PNI commented Jan 26, 2024

Good to go on my side, thank you @danielfmiranda !

Copy link
Contributor

@jhonatan-lopes jhonatan-lopes left a comment

Choose a reason for hiding this comment

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

Hey @danielfmiranda, it's all looking good. I just have one minor correction 🙏

Copy link
Collaborator

@mmmavis mmmavis left a comment

Choose a reason for hiding this comment

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

PR looks good. Just one flag that we need to remove frameborder from iframe as that attribute is obsolete on iframe. We should be able to achieve the same result with Tailwind (whatever the border 0 class is).

@danielfmiranda
Copy link
Collaborator Author

danielfmiranda commented Feb 1, 2024

Thank you both for taking a look! @mmmavis @jhonatan-lopes

I have updated this PR with the following changes, based on your feedback:

  • I have removed the frameborder attribute from the youtube-provided iframe. No additional classes or changes were needed!
  • I have removed {% transblock %} from both NPR quote attributions as the tag was not needed.
  • Regarding the hardcoded URL, after some discussion with Jhonatan, I have updated the template to grab the appropriate URL using {% routablepageurl page 'category-view' slug='best-of' as best_of_url %}. Technically there is still some hardcoding of the slug using this method, but being that this is a one-off page template, and that the users requested this page be updated before the valentines day PNI push, I believe it should do for now.

Edit for @jhonatan-lopes: After testing this out on a review app, it appears that the routablepageurl method was not localizing. To fix this I reverted back to the original URL, however, for the reasons mentioned above, I think it is an OK solution for now as this is a one-off page. 👍

@danielfmiranda danielfmiranda temporarily deployed to foundation-s-feature-11-ha0juw February 1, 2024 01:04 Inactive
Copy link
Collaborator

@mmmavis mmmavis left a comment

Choose a reason for hiding this comment

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

Awesome work! Thanks @danielfmiranda !

@danielfmiranda danielfmiranda temporarily deployed to foundation-s-feature-11-ha0juw February 1, 2024 03:07 Inactive
Copy link
Contributor

@jhonatan-lopes jhonatan-lopes left a comment

Choose a reason for hiding this comment

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

LGTM

@danielfmiranda danielfmiranda temporarily deployed to foundation-s-feature-11-ha0juw February 1, 2024 18:11 Inactive
@danielfmiranda danielfmiranda merged commit 829eda0 into main Feb 1, 2024
6 checks passed
@danielfmiranda danielfmiranda deleted the feature/11138-update-pni-press-page branch February 1, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants