-
Notifications
You must be signed in to change notification settings - Fork 153
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
updated press page content #11766
Conversation
thank you, Daniel!
|
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! |
@mtdenton yes! that's correct. thank you! cc: @danielfmiranda |
Good to go on my side, thank you @danielfmiranda ! |
network-api/networkapi/templates/pages/buyersguide/about/press.html
Outdated
Show resolved
Hide resolved
network-api/networkapi/templates/pages/buyersguide/about/press.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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 🙏
There was a problem hiding this 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).
Thank you both for taking a look! @mmmavis @jhonatan-lopes I have updated this PR with the following changes, based on your feedback:
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. 👍 |
There was a problem hiding this 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 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.