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

Update top read widget to be language variant aware #3895

Merged
merged 8 commits into from
Apr 2, 2021

Conversation

staykids
Copy link
Contributor

@staykids staykids commented Mar 27, 2021

Phabricator: https://phabricator.wikimedia.org/T277697 (this PR is a stepping stone towards addressing this task)

Notes

Previously, the top read widget would get into states where the title and description would be a. displayed in contrasting variants from one another and b. inconsistent with what's displayed in the Explore feed in the app. For example, if you were using Serbian (Latin) as your primary language, you might see a Serbian (Latin) description but a Serbian (Cyrillic) title.

While it's certainly possible you may still see mixed character sets (for backend content reasons), this PR seeks to reach some sense of parity between the top read widget's displayed titles and descriptions and the in-app Explore feed's top read titles and descriptions.

  • Updates the top read widget to use variant aware URLs
  • Populates the widget elements' title and description from the fetched WMFArticle instead of the WMFFeedTopReadArticlePreview

Test Steps

I don't have great test steps here because getting the widgets to reload as desired has always been problematic, but generally:

  1. Make your preferred language one with many variants
  2. Observe the top read widget title and description, ensure they match the in-app Explore feed's top read elements (specifically, that the title and description are using the same variant as the Explore feed's top read group)
  3. Change to a different variant, repeat step 2.

Sometimes you'll encounter cases of the widgets not reloading, in which case sometimes deleting and re-adding the widget helps. The intent of this PR is not so much to address the reloading problem, but to help address the consistency issues of the titles and descriptions as described in step 2.

top_read

@staykids staykids requested review from a team and dempseyatgithub and removed request for a team March 27, 2021 00:03
Copy link
Contributor

@dempseyatgithub dempseyatgithub left a comment

Choose a reason for hiding this comment

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

I had a number of comments.

It also makes me think that I need to create a document with some details about the variant feature, because everything you've done is reasonable, but should be able to be done more simply.

It makes me realize I haven't given the team anything to help you all reason about doing variant-specific things.

Widgets/Widgets/TopReadWidget.swift Outdated Show resolved Hide resolved
WMF Framework/WidgetController.swift Outdated Show resolved Hide resolved
Widgets/Widgets/TopReadWidget.swift Outdated Show resolved Hide resolved
Widgets/Widgets/TopReadWidget.swift Outdated Show resolved Hide resolved
@staykids
Copy link
Contributor Author

staykids commented Apr 1, 2021

Updated and should be ready for another look.

Copy link
Contributor

@dempseyatgithub dempseyatgithub left a comment

Choose a reason for hiding this comment

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

The changes look good!

@dempseyatgithub dempseyatgithub merged commit 00ff7c3 into main Apr 2, 2021
@tonisevener tonisevener deleted the T277697-top-read-widget branch May 24, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants