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

Issue/10623 add dependency attribution #11776

Merged
merged 23 commits into from May 30, 2019

Conversation

@markpar
Copy link

commented May 24, 2019

Fixes #10623.

This fix implements the great suggestion from @jtreanor to convert the 3rd-party acknowledgements markdown created for us by CocoaPods into HTML and then display that in a web view. Using the commonmarker gem, we convert the markdown to HTML during the post_install phase of pod install, adding some lightweight global styling along the way. The resulting HTML is embedded as a resource in the main app bundle as acknowledgements.html and this is then displayed in a web view when the user taps Acknowledgements on the About WordPress for iOS screen.

Unit tests in Xcode (Product > Test) passed.

To test:

  • Boot app.
  • Me > App Settings > About WordPress for iOS > Acknowledgements.

Expect: A list of licenses for third-party libraries we use. Content should be identical to Pods/Target Support Files/Pods-WordPress/Pods-WordPress-acknowledgements.markdown

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.
@markpar

This comment has been minimized.

Copy link
Author

commented May 25, 2019

Here are screenshots for the Acknowledgements entry and the resulting HTML. Styling suggestions welcome!

image

Podfile Outdated Show resolved Hide resolved

@jtreanor jtreanor added this to the 12.6 milestone May 27, 2019

@jtreanor jtreanor self-requested a review May 27, 2019

@megsfulton

This comment has been minimized.

Copy link

commented May 27, 2019

For the rendering of the HTML, just a few small changes to make the text on the page more easily readable. Apologies for not including these details on the original ticket.

  • Add 20px margin/padding to the left, top, and right side so the text isn't against the edge of the screen
  • When the body text is displayed on a mobile browser it should have a font size of 16px and a color of #1A1A1A.

Acknowledgements Redlines

Mark Parris
@markpar

This comment has been minimized.

Copy link
Author

commented May 27, 2019

Thanks so much! Here's a screenshot with the new styling. I used margin instead of explicitly specifying a top, left, and right, which means we also have a bottom margin of 20px. It can easily be changed to explicit values for top, left, and right if we don't want a bottom margin.

image

@jtreanor jtreanor marked this pull request as ready for review May 28, 2019

@jtreanor
Copy link
Contributor

left a comment

From my point of view this looks great now 🎉 Nice work @markpar! There seem to be some conflicts with develop that you will need to resolve before we merge this.

@megsfulton Could you take a quick look at the updates Mark has made to see if they match what you had in mind?

Mark Parris added some commits May 27, 2019

Mark Parris
Merge branch 'issue/10623-add-dependency-attribution' of github.com:m…
…arkpar/WordPress-iOS into issue/10623-add-dependency-attribution
Mark Parris
@megsfulton

This comment has been minimized.

Copy link

commented May 28, 2019

I used margin instead of explicitly specifying a top, left, and right, which means we also have a bottom margin of 20px. It can easily be changed to explicit values for top, left, and right if we don't want a bottom margin.

Having a margin on all sides makes sense!

The text on the screen is still rendering quite small. @markpar could you try 2xing it 32 and see what that looks like?

@markpar

This comment has been minimized.

Copy link
Author

commented May 28, 2019

Sure thing, @megsfulton!

Well, this is weird! I bumped from 16px to 32px and this is the result.
image

When I try an iPad simulator, it looks good. This is 32px, which on iPad looks a little large.
image

Here's 16px on an iPad.
image

I'm wondering if this might be an issue with the iPhone simulator? It appears not to be rendering this web view content correctly, perhaps? My hypothesis is that your initial 16px design was right on the money and the simulator is having a problem. I have a physical iPhone Xr I can try it on, however I'm not on the WP team, so I don't have a provisioning profile and can't deploy it to my device.

@jtreanor, what are your thoughts on how to proceed? I am searching the internets to see if this might be a known issue with WKWebView and local HTML content, and am continuing my attempts to deploy to my physical iPhone.

@markpar

This comment has been minimized.

Copy link
Author

commented May 29, 2019

Turns out the iPhone simulator is, of course, not the problem at all. 🤦‍♂ 😆 There are loads of <pre> tags in the HTML and they don't wrap by default. Adding some CSS to set the value of white-space to pre-wrap restores the wrapping functionality.

After that, there was a single problem child that remained. The license for glog contains this URL http://www.google.com/codesearch/p?hl=en#dR3YEbitojA/COPYING&amp;q=GetSystemTimeAsFileTime%20license:bsd, and it was not wrapping. All the HTML and CSS voodoo I tried did not cause the URL to wrap. I finally resorted to inserting a hardcoded <br> and that did the trick.

Once that was taken care of, I added a meta tag for the viewport and the resulting HTML is the best so far. @megsfulton, would love your feedback!

iPhone
image

iPad
image

@megsfulton

This comment has been minimized.

Copy link

commented May 29, 2019

Looks great now! 🎉

@markpar

This comment has been minimized.

Copy link
Author

commented May 29, 2019

Awesome! Thanks so much! @jtreanor, I think we are ready to merge. 👍 🙂

@jtreanor
Copy link
Contributor

left a comment

Great! Thanks @megsfulton and @markpar.

Merging now 🎉

@jtreanor jtreanor merged commit 482d0bd into wordpress-mobile:develop May 30, 2019

3 checks passed

Hound No violations found. Woof!
Peril All green. Yay.
Details
ci/circleci: build_and_test Your tests passed on CircleCI!
Details

@markpar markpar deleted the markpar:issue/10623-add-dependency-attribution branch Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.