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

Use shared reader css file #12213

Merged
merged 12 commits into from Jun 23, 2020
Merged

Use shared reader css file #12213

merged 12 commits into from Jun 23, 2020

Conversation

malinajirka
Copy link
Contributor

@malinajirka malinajirka commented Jun 17, 2020

Fixes #11673
Fixes #7636
Fixes #11685
Fixes #11779 (I believe it's desired behavior)

The main goal of this PR is to use mobile specific (shared) css file for rendering posts in the Reader. The css file is heavily based on Calypso's Reader css file. This PR also changes some of the hardcoded styles to fix some minor issues.

Questions:

  1. Should/Can we move the hardcoded styles into the shared mobile css file?
  2. As we discussed this implementation uses an infinite cache - it never gets invalidated. Do we want to introduce some automatic cache invalidation or we are okay with an infinite cache which can be invalidated only by manually changing the code?

To test:

  • Test any posts/blocks you can think of.

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.

@malinajirka malinajirka added this to the 15.2 milestone Jun 17, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 17, 2020

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 17, 2020

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

@malinajirka malinajirka added this to In progress in Reader Improvements 📖 Jun 18, 2020
@malinajirka malinajirka moved this from In progress to Review in progress in Reader Improvements 📖 Jun 19, 2020
@malinajirka malinajirka marked this pull request as ready for review June 19, 2020 13:19
Reader Improvements 📖 automation moved this from Review in progress to Reviewer approved Jun 19, 2020
@leandroalonso
Copy link
Contributor

As we discussed this implementation uses an infinite cache - it never gets invalidated. Do we want to introduce some automatic cache invalidation or we are okay with an infinite cache that can be invalidated only by manually changing the code?

For iOS I implemented a class to manage that:

https://github.com/wordpress-mobile/WordPress-iOS/pull/14357/files#diff-33e1d7c630ae0273ff430172c91cc535

I know this is not optimal and ideally, the server should handle that but is enough for now. Although I'm really not sure about the amount of time.

@malinajirka
Copy link
Contributor Author

Thanks for the review!!!

For iOS I implemented a class to manage that:
https://github.com/wordpress-mobile/WordPress-iOS/pull/14357/files#diff-33e1d7c630ae0273ff430172c91cc535

Thanks for the info, I'll implement the same class on Android;). Definitely way better than manual updating:).

@malinajirka malinajirka merged commit c7c46cc into develop Jun 23, 2020
Reader Improvements 📖 automation moved this from Reviewer approved to Done Jun 23, 2020
@malinajirka malinajirka deleted the use-shared-reader-css-file branch June 23, 2020 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
2 participants