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

T248611 Restore "about" feature in article gallery view #195

Merged
merged 11 commits into from
Apr 6, 2020

Conversation

hueitan
Copy link
Member

@hueitan hueitan commented Mar 30, 2020

Phabricator Link: https://phabricator.wikimedia.org/T248611

Problem Statement

pcs media api is being removed recently, switch to mw api

Solution

fetch the api only when user press About center key

Note

About Loading No Image Info
Screen Shot 2020-03-30 at 23 42 24 Screen Shot 2020-03-30 at 23 48 23 Screen Shot 2020-04-01 at 19 46 30

|

@stephanebisson
Copy link
Collaborator

Article en/Brazil, 3rd image has no image info so the popup has only a header.

Screen Shot 2020-03-31 at 06 46 04

@medied
Copy link
Contributor

medied commented Apr 1, 2020

Reviewing this PR was useful for me to further understand how to use cachedFetch and interact with APIs. I tested on Firefox and banana phone and I wasn't able to find any issues

medied
medied previously approved these changes Apr 1, 2020
@hueitan
Copy link
Member Author

hueitan commented Apr 1, 2020

this pr requires more update on the api, see the comment in ticket, in short summary, the image is not always from Commons.

@medied medied dismissed their stale review April 1, 2020 22:20

More changes coming

@hueitan
Copy link
Member Author

hueitan commented Apr 2, 2020

Ok, change is done and ready to be reviewed again, thanks for the input!

title: item.title,
canonicalizedTitle:
item.title && canonicalizeTitle(item.title.split(':')[1])
item.title && canonicalizeTitle(item.title.split(':')[1]),
fromCommon: source.indexOf('/commons') !== -1
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, good idea to declare source at the top

@medied
Copy link
Contributor

medied commented Apr 2, 2020

Tested latest change on banana device using Brasilien article and it worked as expected, btw

@jpita jpita merged commit f7ddef7 into master Apr 6, 2020
@jpita jpita deleted the T248553-media-info branch April 6, 2020 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants