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

feat: implement filterSeenArticles in DCR #8695

Closed
wants to merge 1 commit into from

Conversation

cemms1
Copy link
Contributor

@cemms1 cemms1 commented Aug 30, 2023

What does this change?

Introduces a new prop hasBeenSeen for <Card /> components to allow them to be dimmed for app users who have already visited the article represented by the card.

⚠️ Note that this is very directed towards cards in <Carousel /> components as these are the ones used at the bottom of articles. Any specific fronts-related cards have been left as is since there is no requirement to support fronts for apps.

Prop-drills renderingTarget into a number of components to allow hasBeenSeen to only be truthy for renderingTarget === "Apps"

Why?

As part of the migration of the Bridget user service to DCR, this migrates the use of filterSeenArticles across, since it was being used to dim cards that had already been read by the mobile user.

Potential improvements

The filterSeenArticles method is used in a very convoluted way. In this PR, we aren't interested in changing the underlying Bridget methods so this is being shoehorned in.
It would be worth addressing what the "ideal" method would be here based on what the native layer needs and what makes most sense in this context.

@github-actions
Copy link

Size Change: 0 B 🆕

Total Size: 0 B

compressed-size-action

@cemms1
Copy link
Contributor Author

cemms1 commented Sep 4, 2023

Closing since this is not required for day 1 of DCR for apps project, and this implementation may change after result of #8704

@cemms1 cemms1 closed this Sep 4, 2023
@cemms1 cemms1 deleted the cemms1/bridget-filter-seen branch October 20, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant