Skip to content
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.

Sign up for GitHub

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

maintenance: fixing minor code smells #29706

Open
wants to merge 1 commit into
base: next
Choose a base branch
from
+176 −266
Open

maintenance: fixing minor code smells #29706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 36 additions & 42 deletions code/core/src/manager/FakeProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,41 +6,37 @@ import { addons } from '@storybook/core/manager-api';

import Provider from './provider';

export class FakeProvider extends Provider {
constructor() {
super();

// @ts-expect-error (Converted from ts-ignore)
this.addons = addons;
// @ts-expect-error (Converted from ts-ignore)
this.channel = {
on: () => {},
once: () => {},
off: () => {},
emit: () => {},
addListener: () => {},
removeListener: () => {},
};
}
export const FakeProvider = () => {
// @ts-expect-error (Converted from ts-ignore)
const addonsInstance = addons;
// @ts-expect-error (Converted from ts-ignore)
const channel = {
on: () => {},
once: () => {},
off: () => {},
emit: () => {},
addListener: () => {},
removeListener: () => {},
};
Comment on lines +10 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using a proper mock object or testing utility instead of ts-expect-error and empty function stubs


// @ts-expect-error (Converted from ts-ignore)
getElements(type) {
return addons.getElements(type);
}
const getElements = (type: string) => addonsInstance.getElements(type);

renderPreview() {
return <div>This is from a 'renderPreview' call from FakeProvider</div>;
}
const renderPreview = () => <div>This is from a 'renderPreview' call from FakeProvider</div>;

// @ts-expect-error (Converted from ts-ignore)
handleAPI(api) {
addons.loadAddons(api);
}
const handleAPI = (api: any) => addonsInstance.loadAddons(api);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: api parameter should be properly typed instead of any


const getConfig = () => ({});

getConfig() {
return {};
}
}
return (
<Provider
renderPreview={renderPreview}
getElements={getElements}
handleAPI={handleAPI}
getConfig={getConfig}
/>
);
};

export const Centered = styled.div({
width: '100vw',
Expand All @@ -51,15 +47,13 @@ export const Centered = styled.div({
flexDirection: 'column',
});

export class PrettyFakeProvider extends FakeProvider {
renderPreview(...args: any[]) {
return (
<Centered>
This is from a 'renderPreview' call from FakeProvider
<hr />
'renderPreview' was called with:
<pre>{JSON.stringify(args, null, 2)}</pre>
</Centered>
);
}
}
export const PrettyFakeProvider = (props: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: props should be properly typed instead of any

return (
<Centered>
This is from a 'renderPreview' call from FakeProvider
<hr />
'renderPreview' was called with:
<pre>{JSON.stringify(props, null, 2)}</pre>
</Centered>
);
};
41 changes: 24 additions & 17 deletions code/core/src/manager/components/sidebar/RefBlocks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const TextStyle = styled.div(({ theme }) => ({
lineHeight: '20px',
margin: 0,
}));

const Text = styled.div(({ theme }) => ({
fontSize: theme.typography.size.s2,
lineHeight: '20px',
Expand Down Expand Up @@ -50,24 +51,31 @@ export const AuthBlock: FC<{ loginUrl: string; id: string }> = ({ loginUrl, id }
const [isAuthAttempted, setAuthAttempted] = useState(false);

const refresh = useCallback(() => {
globalWindow.document.location.reload();
setAuthAttempted(false);
}, []);
Comment on lines 53 to 55
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Setting isAuthAttempted to false won't trigger a page refresh. Need to actually reload the page to fetch new data.


const open = useCallback<React.MouseEventHandler>((e) => {
e.preventDefault();
const childWindow = globalWindow.open(loginUrl, `storybook_auth_${id}`, 'resizable,scrollbars');

// poll for window to close
const timer = setInterval(() => {
if (!childWindow) {
logger.error('unable to access loginUrl window');
clearInterval(timer);
} else if (childWindow.closed) {
clearInterval(timer);
setAuthAttempted(true);
}
}, 1000);
}, []);
const open = useCallback<React.MouseEventHandler>(
(e) => {
e.preventDefault();
const childWindow = globalWindow.open(
loginUrl,
`storybook_auth_${id}`,
'resizable,scrollbars'
);

// poll for window to close
const timer = setInterval(() => {
if (!childWindow) {
logger.error('unable to access loginUrl window');
clearInterval(timer);
} else if (childWindow.closed) {
clearInterval(timer);
setAuthAttempted(true);
}
}, 1000);
},
[loginUrl, id]
);

return (
<Contained>
Expand All @@ -79,7 +87,6 @@ export const AuthBlock: FC<{ loginUrl: string; id: string }> = ({ loginUrl, id }
this Storybook.
</Text>
<div>
{/* TODO: Make sure this button is working without the deprecated props */}
<Button small gray onClick={refresh}>
<SyncIcon />
Refresh now
Expand Down
8 changes: 7 additions & 1 deletion code/core/src/manager/components/sidebar/RefIndicator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,8 @@ const SourceCodeMessage: FC<{

const LoginRequiredMessage: FC<RefType> = ({ loginUrl, id }) => {
const theme = useTheme();
const [isLoggedIn, setIsLoggedIn] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: useState import is missing from React imports at the top of the file


const open = useCallback<MouseEventHandler>((e) => {
e.preventDefault();
const childWindow = globalWindow.open(loginUrl, `storybook_auth_${id}`, 'resizable,scrollbars');
Expand All @@ -288,11 +290,15 @@ const LoginRequiredMessage: FC<RefType> = ({ loginUrl, id }) => {
clearInterval(timer);
} else if (childWindow.closed) {
clearInterval(timer);
document.location.reload();
setIsLoggedIn(true);
}
}, 1000);
}, []);

if (isLoggedIn) {
return <div>Login Successful!</div>;
}
Comment on lines +298 to +300
Copy link
Contributor

Choose a reason for hiding this comment

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

style: success message should be styled consistently with other messages using the Message component


return (
<Message onClick={open}>
<LockIcon color={theme.color.gold} />
Expand Down
Loading
Loading