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

Fix #1590: Update licenses file. Add licenses setting option. #1780

Merged
merged 1 commit into from Oct 24, 2019

Conversation

@iccub
Copy link
Contributor

iccub commented Oct 23, 2019

Summary of Changes

This pull request fixes issue #1590

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

Open Settings -> About -> Licenses. Confirm it contains MPL license and licenses of dependencies

Screenshots:

Zrzut ekranu 2019-10-23 o 20 40 04

Zrzut ekranu 2019-10-23 o 20 40 16

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue is assigned to a milestone (should happen at merge time).
@iccub iccub requested review from Brandon-T and jhreis Oct 23, 2019
<input id="ac-11" name="accordion-1" type="checkbox" />
<label for="ac-11"><h2>Alamofire</h2></label>
<input id="ac-Deferred" name="accordion-1" type="checkbox" />
<label for="ac-Deferred"><h2>Deferred</h2></label>

This comment has been minimized.

Copy link
@iccub

iccub Oct 23, 2019

Author Contributor

I ordered dependency licenses to match structure of cartfile

Copy link
Collaborator

Brandon-T left a comment

Hmm it flickers a little when I expand the cells and they have a weird padding but I think this is just due to HTML being rendered instead of native.

Also, if the cell is at the bottom of the screen and I expand, it doesn't push up atm. I guess we can fix this later. For now, code is good.

@iccub iccub added the blocked label Oct 23, 2019
@iccub
Copy link
Contributor Author

iccub commented Oct 23, 2019

Blocking until @jhreis review and/or sec review

@iccub
Copy link
Contributor Author

iccub commented Oct 23, 2019

@Brandon-T yeah, I didn't change the html implementation from what we had from upstream

@jhreis
Copy link
Contributor

jhreis commented Oct 24, 2019

This looks programmatically good to me. I am waiting on some final wording, so not merging right now.

update: received confirmation that this is gtg for now (see #1780 (comment) for more info)

@jhreis
jhreis approved these changes Oct 24, 2019
@jhreis jhreis removed the blocked label Oct 24, 2019
@jhreis jhreis merged commit 9e1b0f2 into development Oct 24, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jhreis jhreis deleted the bugfix/1590 branch Oct 24, 2019
@jhreis
Copy link
Contributor

jhreis commented Oct 24, 2019

We very likely will improve upon this in the future (e.g. better text information, tag linking), but this meets what we need so merging this.

@iccub iccub mentioned this pull request Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.