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

Converts SiteInfo into redux component #9413

Merged
merged 1 commit into from
Jun 14, 2017
Merged

Converts SiteInfo into redux component #9413

merged 1 commit into from
Jun 14, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jun 13, 2017 •
edited by luixxiul
Loading

Test Plan

  • go to https site and check if cert button is visible
  • go to http site and check if text for non verified site is loaded
  • go to new tab and check that site info is not visible

Also: #10931 (comment)

Description

Submitter Checklist:

  • 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).
  • Tagged reviewers and labelled the pull request as needed.

Resolves #9412

Auditors: @bsclifton @bridiver

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@NejcZdovc NejcZdovc added this to the 0.19.x (Nightly Channel) milestone Jun 13, 2017
@NejcZdovc NejcZdovc self-assigned this Jun 13, 2017
@NejcZdovc NejcZdovc force-pushed the redux/siteInfo branch 3 times, most recently from 6d26957 to 7a22995 Compare June 13, 2017 14:34
@NejcZdovc NejcZdovc requested a review from darkdh June 13, 2017 14:38
@NejcZdovc NejcZdovc mentioned this pull request Jun 14, 2017
Closed
51 tasks
@bsclifton
Copy link
Member

bsclifton commented Jun 14, 2017 •
edited
Loading

NOTE: this PR removes the dialog overlay (ex: the shadows behind the modal that is shown). Is this OK, @bradleyrichter?

new look
screen shot 2017-06-14 at 10 08 33 am

old look (notice background is darkened)
screen shot 2017-06-14 at 10 08 48 am

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Jun 14, 2017 •
edited
Loading

@bsclifton This was not changed in this PR. If you checkout the master you will see that background was removed few commits back.

@bsclifton
Copy link
Member

@NejcZdovc yes- you are correct 😄 @luixxiul pointed me at #9034

@bsclifton
Copy link
Member

bsclifton commented Jun 14, 2017 •
edited
Loading

Unrelated to this PR... @darkdh I am seeing the following logged the first time that the show certificate window is displayed:
2017-06-14 10:38:43.553 Brave[80179:5297749] Failed to connect (_okButton) outlet from (SFCertificatePanel) to (NSButton): missing setter or instance variable

(It seems to work as expected though)

bsclifton
bsclifton suggested changes Jun 14, 2017
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Changes look (and work) great! 😄 I left some notes about unit tests which I believe will help prevent regressions. Once there are tests, I think this is good to go 👍

app/renderer/components/main/siteInfo.js

get viewCertificateButton () {
// TODO(Anthony): Hide it until muon support linux
Copy link
Member

Choose a reason for hiding this comment

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

This should have a unit test, IMO. Often times unit tests can document the code as good or better than comments 😄 (the test acting as a specification)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the only thing that we are doing here is checking if linux or not and we can't test getters in redux components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test added

Copy link
Member

Choose a reason for hiding this comment

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

�++

app/renderer/components/main/siteInfo.js Outdated
<div data-l10n-id='secureConnectionInfo' />
{this.viewCertificateButton}
</div>
} else if (this.props.partialySecureConnection) {
Copy link
Member

Choose a reason for hiding this comment

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

s/partialy/partially/g

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

app/renderer/components/main/siteInfo.js

let secureIcon
if (this.isSecure === true && !this.runInsecureContent) {
get secureIcon () {
Copy link
Member

Choose a reason for hiding this comment

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

Now that these are properties, they would be good candidates for unit tests 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as bellow. What we can test is if props are set correctly, so that we move for example isFullySecured into state helper and do a unit test there.

@NejcZdovc NejcZdovc mentioned this pull request Jun 14, 2017
Closed
@bsclifton bsclifton mentioned this pull request Jun 14, 2017
Merged
8 tasks
@darkdh
Copy link
Member

darkdh commented Jun 14, 2017 •
edited
Loading

@bsclifton , it will also happen on the same version of chromium
2017-06-14 12:22:58.559 Chromium[5967:15236306] Failed to connect (_okButton) outlet from (SFCertificatePanel) to (NSButton): missing setter or instance variable
even 61.0.3130.0
2017-06-14 12:24:51.348 Google Chrome Canary[5982:15237832] Failed to connect (_okButton) outlet from (SFCertificatePanel) to (NSButton): missing setter or instance variable

darkdh
darkdh approved these changes Jun 14, 2017
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

�changes looks good to me

bsclifton
bsclifton approved these changes Jun 14, 2017
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Waiting for travis to finish, but updates look great! 😄 ++

@NejcZdovc NejcZdovc merged commit 20d570b into brave:master Jun 14, 2017
@luixxiul luixxiul mentioned this pull request Sep 13, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@darkdh darkdh darkdh approved these changes

@bsclifton bsclifton bsclifton approved these changes

@bridiver bridiver Awaiting requested review from bridiver

Assignees

@NejcZdovc NejcZdovc

Labels
QA/test-plan-specified refactoring/redux-component release-notes/exclude
Projects
None yet
Milestone
0.19.x (Beta Channel)
Development

Successfully merging this pull request may close these issues.
3 participants
@NejcZdovc @bsclifton @darkdh