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

New privacy-footer-localiser component #842

Closed
wants to merge 12 commits into from
Closed

Conversation

carlesandres
Copy link
Contributor

@carlesandres carlesandres commented Jul 1, 2020

This implements the new dotcom-privacy-footer-localiser package as discussed in #840

@carlesandres carlesandres marked this pull request as ready for review July 1, 2020 15:43
@carlesandres carlesandres requested a review from a team as a code owner July 1, 2020 15:43
Copy link
Contributor

@oliverturner oliverturner left a comment

Choose a reason for hiding this comment

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

This is all looking A+, but could I ask you to provide an implementation for /examples?

@carlesandres
Copy link
Contributor Author

carlesandres commented Jul 2, 2020

@oliverturner I sure can do. Would you favour a brand new example or reusing an existing one? Considering there is missing CCPA functionality waiting for this new component, is it worth holding the release off until we have the example?

@carlesandres carlesandres changed the title Footer localiser New privacy-footer-localiser component Jul 2, 2020
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Hello :)

I know you're under time constraints so my comments are non-blocking and we could consider them after this ships.

If the privacy stuff is across all of FT.com could this functionality be in the footer component itself?

I worry that longer term this package is relying on internal implementation details of the footer which could break:

"#site-footer [href='http://help.ft.com/help/legal-privacy/privacy"

If there's a good reason for having them separate, could we consider having some sort of explicit way for the footer to expect changes rather than modifying the output of the footer?

As Andy has mentioned elsewhere I wonder if having these links in by default and using JavaScript to hide them if necessary might be a simpler approach?

You could still have the linked pages themselves controlled by regions without worrying too much about CDN caching being split.

@carlesandres
Copy link
Contributor Author

@NickColley thanks for having a look. You are raising very valid points here and I believe that, one way or another, we've tried to address them all in this issue: #840. Let me know if you need further clarification after reading that.

The way I see it, there's very little we can do to mitigate our dependency on that DOM markup other than:

  1. Reserving an area in the footer for Privacy that the Ads & Privacy team can own as per @oliverturner 's suggestion. As far as I can see, this would not be a trivial task because the footer is populated via next-navigation-api/next-api.
  2. Actively monitor the presence of that CSS selector in the footer, which we already do via our test-in-production bot n-live-ads-testing which runs nightly.

@NickColley
Copy link
Contributor

@carlesandres could we have some sort of logic that makes similar checks to what you've proposed when iterating over the result of the navigation api?

https://github.com/Financial-Times/dotcom-page-kit/blob/master/packages/dotcom-ui-footer/src/components/partials.tsx#L62

I don't understand these things well enough to offer any other suggestions, thanks for your patience.

@kavanagh
Copy link
Member

kavanagh commented Jul 3, 2020

@carlesandres I somehow missed the last few messages in the proposal discussion. I thought you were still mulling this over. I'll go and read the thread and add comments there before I look at this.

How long until this deadline? I'd prefer we get this right than undo stuff after.

@kavanagh
Copy link
Member

kavanagh commented Jul 3, 2020

I worry that longer term this package is relying on internal implementation details of the footer which could break:

"#site-footer [href='http://help.ft.com/help/legal-privacy/privacy"

If there's a good reason for having them separate, could we consider having some sort of explicit way for the footer to expect changes rather than modifying the output of the footer?

As Andy has mentioned elsewhere I wonder if having these links in by default and using JavaScript to hide them if necessary might be a simpler approach?

I agree with all of this and think we should get this right before releasing the changes if it means merging loads of Renovate PRs twice. Also, it looks like you might have some issues with the PageKit major version moving to 2. Lots of Apps are on version 1 or below. Is that right?

Could all the markup live in the footer component with each privacy element/link having an attribute for the relevant compliance region. Then this module could be responsible for hiding them?

eg

<-- in footer -->
<a href="whatevs" data-privacy-link="ccpa">Don't sell my data</a>
// in footer-localiser
const element = document.querySelector('data-privacy-link["ccpa"]');
element.classList.add('privacy--not-applicable');

@carlesandres
Copy link
Contributor Author

@kavanagh Since there weren't any fully-fledged competing proposals and we are past the ideal deadline of July 1st, I thought it would be best to propose an initial implementation of a new PageKit component, since I understood you, @andygout and @oliverturner were pretty aligned in bringing this functionality to PageKit.

Since Compliance only told us about the requirement to change the "Privacy" link text for CA users last week, we've been given until July 7th, although the functionality should have been live since July 1st ideally.

With regards to changing the "Do Not Sell MY Info"" link to render server-side (this is already added client-side in production), I believe it makes things harder as it requires changes in next-navigation (which I am not sure how it works at the moment). As I mentioned in #840 , I'm happy to work on any enhancements/refinements you want to suggest but since my team is quite stretched at the moment, I might need to discuss priorities before I can work on them.

@carlesandres
Copy link
Contributor Author

@NickColley I am not sure I understand your proposal fully. Maybe we could jump on a quick call with @kavanagh on Monday if you both think you can find the time?

@carlesandres carlesandres requested a review from andygout July 6, 2020 09:41
@carlesandres
Copy link
Contributor Author

@kavanagh @andygout Thanks for your time earlier. I got the green light to change "Privacy Policy" to "Privacy - CCPA UPDATES" for all users 🎉 So I made this PR: https://github.com/Financial-Times/origami-navigation-data/pull/257/files . It only needs to be like that until 1/8/2020 and then we will revert it.

If you figure out an acceptable architecture to make privacy-related DOM manipulations to the footer, I'd be glad to know all about it.

I'll keep this PR open for a few more days in case you want to add something to it.

@carlesandres
Copy link
Contributor Author

@kavanagh @andygout @NickColley @oliverturner I am tempted to close this PR since it doesn't seem we are likely to find the time to reach an agreement on dynamic changes to the DOM soon.

So far, it has been a bit of a waste of time and I believe we might still have to resuscitate this topic in the future, but we are good for now. Any thoughts?

@andygout
Copy link
Contributor

andygout commented Aug 12, 2020

If the topic does need to be resuscitated in the future the specifications may have been modified which would potentially require us to re-think the approach further.

As we cannot foresee what future modifications might be required I feel that we should close this PR in the meantime but that in the scenario of the work returning it would likely serve as a very useful reference. As such, I don't feel we should necessarily consider the work and discussions done to date as a waste of time; the exploration of ideas is always going to be necessary to find a solution and those form part of that process.

@carlesandres
Copy link
Contributor Author

@andygout I am glad you see that way. I was frustrated that I might have overthought all of this and dragged you all along into a useless discussion.

I'll close it now and, as you say, we can use it as a reference in the future if needed.

Thank you all for your feedback. I think very valid points were made by everyone in the discussion.

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