-
Notifications
You must be signed in to change notification settings - Fork 971
Made urlBarIconContainer clickable to display connection info #13164
Conversation
@MargarytaChepiga can you please review this PR? @srirambv had some tweaks to your original contribution 😄 BTW- you should have an invitation to be an official collaborator now. Once you accept, you can assign yourself to issues 😄 |
@MargarytaChepiga (and everybody else) - apologies for what happened here. I didn't do a proper review when accepting the original pull request (definitely my bad). Part of a good review includes checking the Travis CI status, which I neglected to do For sensitive areas like the URL bar, it's also great to loop in security. To remedy this, I rolled back the code in master... re-introduced it here... and then created a security review 😄 |
Codecov Report
@@ Coverage Diff @@
## master #13164 +/- ##
==========================================
- Coverage 56.18% 55.92% -0.26%
==========================================
Files 279 281 +2
Lines 27873 27855 -18
Branches 4560 4571 +11
==========================================
- Hits 15660 15579 -81
- Misses 12213 12276 +63
|
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.
Everything looks great, besides that extra space in the icon caused by padding. Since this issue was a part of the reverted merge, can I make this change?
less/navigationBar.less
Outdated
@@ -835,7 +832,7 @@ | |||
background-position: center; | |||
position: relative; | |||
bottom: -1px; | |||
margin-left: 3px; | |||
padding: 5px 7px 5px 7px; |
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.
The extra space between an icon and address is caused by the padding. Looked okay with the secure connection, however as @diracdeltas mentioned with others not really. Changing to 5px 5px 5px 5px makes it better.
@MargarytaChepiga yes, please feel free to make that change! to have it be part of this PR, either:
or
|
@MargarytaChepiga please go ahead and make the changes. I've added you to the branch |
Sorry for the delay. I'm facing issues pushing the changes. Probably I am doing something wrong. |
@MargarytaChepiga I think its due to the fact that its still not given you access. Here's the link https://github.com/srirambv/browser-laptop/invitations . Could you please try once again |
@srirambv My bad, thank you so much! |
Thanks, this looks great. However it doesn't seem to fix #13011 for me, not sure why. When I click on the EV certificate info on github.com, nothing happens unless I click on the lock icon. |
@diracdeltas That's weird... I just checked it again, just to be sure. It seems to work fine. |
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.
nvm, it works now. lgtm!
Fixes #7888
Redo of #13147 (which unintentionally introduced #13163)
Submitter Checklist:
git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests