-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add lock for private orgs on own profile #1490
Add lock for private orgs on own profile #1490
Conversation
I think just making the inner part of the lock non-transparent (e.g. white or very light grey) would make this quite visually acceptable :) |
@waldyrious Thanks for the suggestion! |
Looks much better, nice work! I think a small detail that could improve the visibility further is a thin white outline so that the shape of the lock is not lost in darker backgrounds. I would also not fill in the inside of the lock's arc (but would add the outline there too). |
Here's a suggestion: <svg xmlns="http://www.w3.org/2000/svg" height="16.426" width="13.426" viewBox="-118 118 13.426 16.426">
<path d="M-111.287 118c-2.59 0-4.713 2.124-4.713 4.713V124h-.287c-.488 0-.892.222-1.192.521-.3.3-.521.704-.521 1.192v7c0 .488.222.892.521 1.191.3.3.704.522 1.192.522h10c.488 0 .892-.222 1.191-.522.3-.3.522-.703.522-1.191v-7c0-.488-.222-.892-.522-1.192-.3-.3-.703-.521-1.191-.521h-.287v-1.287c0-2.59-2.124-4.713-4.713-4.713zm0 3.227c.804 0 1.486.682 1.486 1.486V124h-2.972v-1.287c0-.804.682-1.486 1.486-1.486z" fill="#fff"/>
<path d="M-106.287 124.713h-1v-2c0-2.2-1.8-4-4-4s-4 1.8-4 4v2h-1c-.5 0-1 .5-1 1v7c0 .5.5 1 1 1h10c.5 0 1-.5 1-1v-7c0-.5-.5-1-1-1zm-7.2-2c0-1.2 1-2.2 2.2-2.2 1.2 0 2.2 1 2.2 2.2v2h-4.4z" fill="#333"/>
<path d="M-115.287 132.713h9v-7h-9v7zm1-6h1v1h-1v-1zm0 2h1v1h-1v-1zm0 2h1v1h-1v-1z" fill="#eee"/>
<path fill="#333" d="M-114.287 130.713h1v1h-1zM-114.287 126.713h1v1h-1zM-114.287 128.713h1v1h-1z"/>
</svg> I've uploaded it to svgshare, which conveniently shows how it looks on a variety of backgrounds: |
source/features/mark-private-orgs.js
Outdated
import api from '../libs/api'; | ||
|
||
export default async () => { | ||
if (!getCleanPathname().startsWith(getUsername())) { |
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.
I think this should be added to https://github.com/sindresorhus/refined-github/blob/30071048dd10a1b12ade945c198951e262199631/source/libs/page-detect.js as isOwnUserProfile
readme.md
Outdated
@@ -135,6 +135,7 @@ GitHub Enterprise is also supported. More info in the options. | |||
- [Review counts are shown and colored in PR lists.](https://user-images.githubusercontent.com/1402241/33474535-a814ee78-d6ad-11e7-8f08-a8b72799e376.png) | |||
- [All available keyboard shortcuts are shown in the help modal.](https://user-images.githubusercontent.com/29176678/36999174-9f07d33e-20bf-11e8-83e3-b3a9908a4b5f.png) *(<kbd>?</kbd> hotkey)* | |||
- [Followers you know are shown on profile pages.](https://user-images.githubusercontent.com/2906365/42009293-b1503f62-7a57-11e8-88f5-9c2fb3651a14.png) | |||
- [Private organizations are marked on your profile.](https://user-images.githubusercontent.com/6775216/44633467-d5dcc900-a959-11e8-9116-e6b0ffef66af.png) |
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.
Should be clear this is only visible when you are viewing your profile.
source/features/mark-private-orgs.js
Outdated
} | ||
// List public orgs | ||
const publicOrgs = []; | ||
const orgDataList = await api(`users/${getUsername()}/orgs`, true); |
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.
It would be better to get the information whether the org is private or not from the JSON payload, rather than doing an anonymous request. And authorized request has other benefits, like much higher rate limit, so we should prefer that whenever possible.
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.
Turns out this endpoint returns the public orgs, whether or not the user is signed in. No need for anon requests.
source/libs/api.js
Outdated
@@ -2,7 +2,7 @@ import OptionsSync from 'webext-options-sync'; | |||
|
|||
const cache = new Map(); | |||
|
|||
export default async endpoint => { | |||
export default async (endpoint, anonymousRequest = false) => { |
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.
Use an options-object instead of a boolean argument.
source/features/mark-private-orgs.js
Outdated
publicOrgs.push('/' + orgData.login); | ||
} | ||
|
||
// Display |
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 comment is not useful. Display
doesn't mean anything without any context.
source/features/mark-private-orgs.js
Outdated
import * as icons from '../libs/icons'; | ||
import api from '../libs/api'; | ||
|
||
export default async () => { |
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.
Use a normal non-arrow async function for consistency with other features.
source/libs/icons.js
Outdated
|
||
export const privateLock = () => <svg className="octicon octicon-lock" viewBox="0 0 12 16" version="1.1" width="12" height="16" aria-hidden="true"><path fill-rule="evenodd" d="M4 13H3v-1h1v1zm8-6v7c0 .55-.45 1-1 1H1c-.55 0-1-.45-1-1V7c0-.55.45-1 1-1h1V4c0-2.2 1.8-4 4-4s4 1.8 4 4v2h1c.55 0 1 .45 1 1zM3.8 6h4.41V4c0-1.22-.98-2.2-2.2-2.2-1.22 0-2.2.98-2.2 2.2v2H3.8zM11 7H2v7h9V7zM4 8H3v1h1V8zm0 2H3v1h1v-1z" /></svg>; | ||
|
||
export const privateLockFilled = () => |
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.
Would be better to accept a size
argument here instead of using CSS transform.
A test is failing because of the |
source/libs/icons.js
Outdated
export const privateLockFilled = () => | ||
<svg version="1.1" className="octicon octicon-lock" height="16.426" width="13.426" viewBox="-118 118 13.426 16.426"> | ||
export const privateLockFilled = (width, height) => | ||
<svg version="1.1" className="octicon octicon-lock" height={height} width={width} viewBox={`-118 118 ${width} ${height}`}> |
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 is not correct. viewBox
should not be touched.
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.
And it should be a size
argument with one value that is used for both width/height. The viewBox should ensure to keep the aspect ratio.
readme.md
Outdated
@@ -135,6 +135,7 @@ GitHub Enterprise is also supported. More info in the options. | |||
- [Review counts are shown and colored in PR lists.](https://user-images.githubusercontent.com/1402241/33474535-a814ee78-d6ad-11e7-8f08-a8b72799e376.png) | |||
- [All available keyboard shortcuts are shown in the help modal.](https://user-images.githubusercontent.com/29176678/36999174-9f07d33e-20bf-11e8-83e3-b3a9908a4b5f.png) *(<kbd>?</kbd> hotkey)* | |||
- [Followers you know are shown on profile pages.](https://user-images.githubusercontent.com/2906365/42009293-b1503f62-7a57-11e8-88f5-9c2fb3651a14.png) | |||
- [Private organizations are marked on your own profile.](https://user-images.githubusercontent.com/6775216/44633467-d5dcc900-a959-11e8-9116-e6b0ffef66af.png) |
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.
Private organizations are marked when viewing your own profile.
|
Actually, the error appears to come from the import statement: import {getUsername} from './utils';
Could the |
I think you can just mock |
The |
source/libs/utils.js
Outdated
|
||
export const metaKey = isMac ? 'metaKey' : 'ctrlKey'; | ||
export const metaKey = () => isMac() ? 'metaKey' : 'ctrlKey'; |
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 is not affecting the tests.
source/libs/utils.js
Outdated
@@ -142,9 +142,9 @@ export const flatZip = (table, limit = Infinity) => { | |||
return zipped; | |||
}; | |||
|
|||
export const isMac = /Mac/.test(window.navigator.platform); | |||
export const isMac = () => /Mac/.test(window.navigator.platform); |
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.
I prefer not having to change code because of testing. I already commented a way to fix this properly.
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.
Like I said, the issue itself isn't because of a missing mock for the platform property, but rather that the window
global isn't initialized when the utils
file is imported.
import test from 'ava';
import * as pageDetect from '../source/libs/page-detect'; // Imports utils.js, error is thrown because no window global
import Window from './fixtures/window';
// global mocks
global.window = new Window();
global.location = window.location;
global.document = {};
I tried creating a new file which sets the window
global mocks, and is imported before the page-detect
file. That didn't appear to solve the issue, as the utils
file is being imported from somewhere else (?)
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.
Tests should be fixed properly now, and I reverted the workaround.
e: looks like some more things need to be mocked
This is looking good now. Thanks, @Momothereal :) |
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Now that this is merged to |
A new release is automatically published every 24 hours (if there are new commits). |
@sindresorhus Aha got it. Thanks! |
Adds a little "lock" octicon for private organizations on a user's own profile.
UI is kinda ew, suggestions welcome!
Example: