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.

Sign up for GitHub

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

Jump to bottom

Improve copyright statement #3828

Merged
merged 2 commits into from
Nov 5, 2019
Merged

Improve copyright statement #3828

merged 2 commits into from
Nov 5, 2019

Conversation

fmarier
Copy link
Member

@fmarier fmarier commented Oct 30, 2019 •
edited
Loading

Fixes brave/brave-browser#6152 and brave/brave-browser#6605.

Copy approved by @tomlowenthal.

Submitter Checklist:

Test Plan:

Open chrome://settings/help and verify that:

  • Version number is clickable and links to the release notes
  • Copyright statement includes a working link to the MPL
  • Copyright statement includes a link to the build instructions and GitHub tag
  • Link to chrome://credits for third-party Open Source software is working.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@fmarier fmarier added this to the 0.73.x - Nightly milestone Oct 30, 2019
@fmarier fmarier self-assigned this Oct 30, 2019
@fmarier fmarier force-pushed the francois-improve-copyright-statement branch from 4fb938f to 6e98f90 Compare October 30, 2019 21:56
@fmarier fmarier force-pushed the francois-improve-copyright-statement branch from 6e98f90 to 16606f0 Compare October 30, 2019 22:31
@fmarier fmarier requested a review from bridiver October 30, 2019 22:41
bsclifton
bsclifton reviewed Oct 30, 2019
View reviewed changes
chromium_src/chrome/browser/ui/webui/settings/about_handler.cc Outdated
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */
* You can obtain one at https://mozilla.org/MPL/2.0/. */
Copy link
Member

Choose a reason for hiding this comment

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

💯

bsclifton
bsclifton approved these changes Oct 30, 2019
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Really nice work! I didn't double check any of the GN deps though 🙀 (I think you just moved them around) but the functionality works great

It might be possible to do the patch for the help page using some of what @petemill did - let me try and find an example

petemill
petemill requested changes Oct 30, 2019
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

Please remove the patch, this can be done via the template modification method provided in the comment.

Also please update either the PR description or the issue description with what the change actually is or should be. The issue mentions that there should be "a link somewhere" but doesn't explain what the placement (or design) should be, or what the text should be.

patches/chrome-browser-resources-settings-about_page-about_page.html.patch Outdated
</a>
</div>
</if>
- <div class="secondary">$i18n{aboutBrowserVersion}</div>
Copy link
Member

Choose a reason for hiding this comment

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

This can be achieved via template modification in brave_settings_overrides.js

Something like:

{
...
'settings-about-page': (templateContent) => {
  templateContent.querySelector('#updateStatusMessage .secondary).innerHTML = loadTimeData.getString('aboutBrowserVersion')
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@petemill Is https://github.com/brave/brave-core/pull/3828/files#diff-166e28418b661667403b64249ce36c3a what you had in mind?

@fmarier
Copy link
Member Author

fmarier commented Oct 31, 2019

Also please update either the PR description or the issue description with what the change actually is or should be. The issue mentions that there should be "a link somewhere" but doesn't explain what the placement (or design) should be, or what the text should be.

Good idea. The issue has a mockup but I'll add a text description to the commit message.

@petemill
Copy link
Member

Good idea. The issue has a mockup but I'll add a text description to the commit message.

oh ha, I didn't realize that was a mockup - I thought the line was drawing attention to what needed to be changed, not to what the final change should look like! 😆

@rebron
Copy link
Collaborator

rebron commented Oct 31, 2019

brave/brave-browser#6152 (comment) @petemill @fmarier normal styling for that link is fine. Doesn't need to be the underline and should mimic other link styles on that page.

@fmarier
Copy link
Member Author

fmarier commented Oct 31, 2019

brave/brave-browser#6152 (comment) @petemill @fmarier normal styling for that link is fine. Doesn't need to be the underline and should mimic other link styles on that page.

Yes, I kept the default styling:
Screenshot from 2019-10-31 10-51-54

@fmarier fmarier force-pushed the francois-improve-copyright-statement branch 2 times, most recently from bc69879 to 996dc13 Compare October 31, 2019 23:24
@fmarier fmarier requested a review from petemill October 31, 2019 23:25
@fmarier
Improve licensing notice (fixes brave/brave-browser#6605)
77a3f3e 
This add a link to the license, build instructions as well as the
corresponding source code.
@fmarier
Link to release notes from brave://settings/help (fixes brave/brave-b…
0963172 
…rowser#6152)

The version number on the about page should be a link to the page
for the latest release notes.
@fmarier fmarier force-pushed the francois-improve-copyright-statement branch from 996dc13 to 0963172 Compare November 5, 2019 19:18
petemill
petemill approved these changes Nov 5, 2019
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for addressing feedback

@fmarier fmarier merged commit 061b7f1 into master Nov 5, 2019
@fmarier fmarier deleted the francois-improve-copyright-statement branch November 5, 2019 23:12
@fmarier fmarier modified the milestones: 0.73.x - Dev, 0.74.x - Nightly Nov 5, 2019
This was referenced Nov 9, 2019
Merged
Merged
Merged
@GeetaSarvadnya GeetaSarvadnya mentioned this pull request Nov 11, 2019
Closed
@fmarier fmarier mentioned this pull request Nov 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

@petemill petemill petemill approved these changes

@bsclifton bsclifton bsclifton approved these changes

Assignees

@fmarier fmarier

Labels
None yet
Projects
None yet
Milestone
1.3.x - Release
Development

Successfully merging this pull request may close these issues.

Add link to brave.com/latest in "About Brave" section
5 participants
@fmarier @petemill @rebron @bridiver @bsclifton