Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Add basic styles to about:about and created page w/ version information (about:brave) #5436

Merged
merged 4 commits into from
Nov 7, 2016
Merged

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Nov 6, 2016

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Add basic styles and version information to about:about

Fixes #463

Auditors: @bbondy, @bradleyrichter

Test Plan

  1. Launch Brave and visit about:about
  2. Pull up versions from the "About Brave" message box: Help > About Brave (or on Mac, Brave > About Brave)
  3. Compare the versions shown on the about:about page to the ones in the message box

Screenshots

The new look
screen shot 2016-11-06 at 2 20 12 am

Text is really easy to select for copy/paste
screen shot 2016-11-06 at 2 22 27 am

compare this to the current look
screen shot 2016-11-06 at 2 35 14 am

super()
this.state = { versionInformation: Immutable.fromJS([]) }
ipc.on(messages.VERSION_INFORMATION_UPDATED, (e, versionInformation) => {
console.log('got message')
Copy link
Member

Choose a reason for hiding this comment

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

nit remove logging

ipc.on(messages.VERSION_INFORMATION_UPDATED, (e, versionInformation) => {
console.log('got message')
if (this.state.versionInformation.size === 0) {
console.log('updating')
Copy link
Member

Choose a reason for hiding this comment

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

nit: logging

@bbondy
Copy link
Member

bbondy commented Nov 7, 2016

This is great, I don't mind about:about being combined either. Can you get rid of the dialog though from the about menu and make it go here instead? Test main menu locations and hamburger menu location.

@bsclifton
Copy link
Member Author

Maybe once there's more about pages, we can break this out into it's own about:version page; my initial thought was that (with limited info) it makes sense to show as many details about Brave as possible in one place 😄 We could possibly add more info we find valuable for troubleshooting, like OS version, Video drivers, etc.

Working on the fixes now...

@luixxiul
Copy link
Contributor

luixxiul commented Nov 7, 2016

Wouldn't it nice if we display the additional version info like "RC1" or "preview1"?

@bsclifton
Copy link
Member Author

bsclifton commented Nov 7, 2016

Good call, @luixxiul! I captured that with #5462. For many reasons, it would be more useful to use the last SHA from git rather than an arbitrary string (one example: the release candidate gets renamed when accepted)

@bsclifton
Copy link
Member Author

bsclifton commented Nov 7, 2016

Updated!

@darkdh, if you have a moment, please check out the changes. There are two message boxes that were in app/aboutDialog.js which I moved into app/importer.js. They were being exported, but I changed them to local const because they're not used elsewhere. I tested an import to verify it loads (but didn't try the one w/ a warning).

@bbondy picking About Brave now brings you to the about page 😄 The previous message box has been removed

@bsclifton
Copy link
Member Author

bsclifton commented Nov 7, 2016

Here we go- even more useful info for debugging 😄

screen shot 2016-11-07 at 11 34 30 am

Muon being our fork of electron

@darkdh
Copy link
Member

darkdh commented Nov 7, 2016

++ for the importer dialog, thx.

@bbondy
Copy link
Member

bbondy commented Nov 7, 2016

cc @bradleyrichter do we want to combine the about page with about:about? Note other browsers keep them separate and we might double or triple our about:about pages in the next year. I'm fine either way.

Removed app/aboutDialog.js, moved import methods over to `app/importer.js`
Added os platform/release/arch and changed electron to muon :)

Fixes #463

Auditors: @bbondy, @bradleyrichter, @darkdh (see changes to app/importer.js)

Test Plan:
1. Launch Brave and visit about:about
2. Pull up version from Help > About Brave (or on Mac, Brave > About Brave)
3. Import bookmarks to ensure the message box shown there still works
@bsclifton bsclifton changed the title Add basic styles and version information to about:about Add basic styles to about:about and created page w/ version information (about:brave) Nov 7, 2016
break
case 'about:safebrowsing':
element = require('./safebrowsing')
break
case 'about:styles':
element = require('./styles')
break
Copy link
Member Author

Choose a reason for hiding this comment

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

this is sorted alphabetical now

'about:passwords': module.exports.getAppUrl('about-passwords.html'),
'about:flash': module.exports.getAppUrl('about-flash.html'),
'about:error': module.exports.getAppUrl('about-error.html'),
'about:autofill': module.exports.getAppUrl('about-autofill.html'),
'about:styles': module.exports.getAppUrl('about-styles.html')
Copy link
Member Author

Choose a reason for hiding this comment

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

same here; alphabetical FTW 😛

@bbondy
Copy link
Member

bbondy commented Nov 7, 2016

fancy with the sortable table 👍

@bbondy
Copy link
Member

bbondy commented Nov 7, 2016

fannnntastic, thanks.

@bbondy bbondy merged commit 9bff90a into brave:master Nov 7, 2016
@bsclifton bsclifton deleted the about branch November 8, 2016 00:13
@srirambv
Copy link
Collaborator

srirambv commented Nov 8, 2016

@bsclifton doesn't brave/muon/channel/os needs to be capitalized??

@bsclifton
Copy link
Member Author

@srirambv that is a good question! I didn't consider that- I'll do a follow up which fixes that 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants