-
Notifications
You must be signed in to change notification settings - Fork 891
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
Improve copyright statement #3828
Conversation
patches/chrome-browser-resources-settings-about_page-about_page.html.patch
Outdated
Show resolved
Hide resolved
4fb938f
to
6e98f90
Compare
6e98f90
to
16606f0
Compare
* 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/. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
There was a problem hiding this comment.
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
There was a problem hiding this 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.
</a> | ||
</div> | ||
</if> | ||
- <div class="secondary">$i18n{aboutBrowserVersion}</div> |
There was a problem hiding this comment.
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')
}
There was a problem hiding this comment.
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?
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! 😆 |
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. |
|
bc69879
to
996dc13
Compare
This add a link to the license, build instructions as well as the corresponding source code.
…rowser#6152) The version number on the about page should be a link to the page for the latest release notes.
996dc13
to
0963172
Compare
There was a problem hiding this 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
Fixes brave/brave-browser#6152 and brave/brave-browser#6605.
Copy approved by @tomlowenthal.
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Open
chrome://settings/help
and verify that:chrome://credits
for third-party Open Source software is working.Reviewer Checklist:
After-merge Checklist:
changes has landed on.