Skip to content

Reader: Support Stories in detail view - #13002

Merged
malinajirka merged 3 commits into
developfrom
feature/stories-reader-detail
Sep 24, 2020
Merged

Reader: Support Stories in detail view#13002
malinajirka merged 3 commits into
developfrom
feature/stories-reader-detail

Conversation

@aforcier

@aforcier aforcier commented Sep 22, 2020

Copy link
Copy Markdown
Contributor

Tweaks the Reader to support the Story block in detail view:

device-2020-09-22-122031

The bulk of the work for this has been done in the shared CSS file in Calypso and already deployed: Automattic/wp-calypso#45397

Extra changes that were needed for WPAndroid:

  • The in-app custom CSS forces auto height/width for divs, which breaks with the Story markup. Stories already have set, mobile-friendly width/height params, so I excluded Stories from the rule.
  • I excluded Stories from the img tag re-building we do, as it strips class attributes important for the CSS. Again, we can just rely on the attributes provided by the Story markup.
  • Tapping on a story directly opens the external browser instead of the in-app web viewer. This is because the in-app web viewer has a lot of navigational chrome and doesn't provide as a good a 'full-screen' experience as the browser does.

iOS PR: wordpress-mobile/WordPress-iOS#14965

To test:

  1. Proxy WP.com on your device (because the Story block is currently a beta block and poorly supported by the API)
  2. Clear device data to make sure you get a fresh copy of the reader-mobile.css and aren't hit by the 5-day cache
  3. Follow a site or two with Stories (DM me for some samples)
  4. Check that opening a story in Reader detail shows a properly formatted story, like the screenshot above
  5. Check that tapping the story opens it in the browser, and it automatically plays once the page loads

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Specifically, this keeps class tags which we need for the
Stories CSS to be applied. Otherwise, the story's preview
image isn't centered vertically if it's shorter than
the story container.
@peril-wordpress-mobile

Copy link
Copy Markdown

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile

Copy link
Copy Markdown

You can test the changes on this Pull Request by downloading the APK here.

@malinajirka malinajirka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @aforcier! It all looks great overall. I've encountered one small issue though. When I click on a story on a .com site the system prompts me to choose an app I want to open it in -> when I choose WordPress Android it just re-opens Reader Post Detail view. I'm not sure we can modify the intent filter somehow. Wdyt?

@aforcier

Copy link
Copy Markdown
Contributor Author

Yikes, good catch - I'm not sure why this doesn't trigger on my emulators, but I can confirm it on an actual device.

Looks like the intent filter needs to be modified, yes - I'll look into it and follow up, thanks 👍

@aforcier

aforcier commented Sep 24, 2020

Copy link
Copy Markdown
Contributor Author

So here's what I've found out.

We already disable the Reader deeplink when opening an external URL, so that the app doesn't appear as an option. We then re-enable it (there and in a couple of other places for safety).

As far as I can tell, on certain APIs and devices the re-enabling of the Reader deeplink component in that finally block happens before the startActivity call resolves the intent, so the Reader component which was removed is readded to the list.

This affects the Reader deeplinks in general (try hitting the globe button in the detail page instead for example) and isn't specific to Stories, but affects Stories a bit worse since they effectively link to themselves.

Reproduced on:
A Pixel 3a emulator, API 29 (@malinajirka's report above)
A Pixel 3 device, API 30

Could not reproduce on:
A Pixel 3XL emulator, API 29
A Pixel 3 emulator, API 28

I'll open an issue and resolve this in a separate PR - I think the change will be somewhat deep and I don't want to bury it in an apparently unrelated PR.

Edit: Filed in #13010, fixed in #13012.

@malinajirka malinajirka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, LGTM and I can confirm the fix works as expected.

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.

2 participants