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.

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

feat: add placements #8213

Merged
merged 51 commits into from
Feb 15, 2023
Merged

feat: add placements #8213

merged 51 commits into from
Feb 15, 2023

Conversation

fiji-flo
Copy link
Contributor

No description provided.

@github-actions github-actions bot added dependencies Pull requests that update a dependency file deployer Deployment (currently using AWS S3 and AWS Lambda) github-actions plus work around features related to MDN Plus labels Feb 14, 2023
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

First round of review (until client/src/setupProxy.js)

@@ -231,6 +234,10 @@ jobs:

DEPLOYER_LAMBDA_PREFIX: stage-

KEVEL_SITE_ID: ${{ secrets.DEPLOYER_STAGE_KEVEL_SITE_ID }}
KEVEL_NETWORK_ID: ${{ secrets.DEPLOYER_STAGE_KEVEL_NETWORK_ID }}
SIGN_SECRET: ${{ secrets.DEPLOYER_STAGE_SIGN_KEY }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This sign secret not specific to Kevel?

client/src/advertisement/about/index.tsx Outdated Show resolved Hide resolved
client/src/advertisement/about/index.tsx Outdated Show resolved Hide resolved
client/src/advertisement/about/index.tsx Outdated Show resolved Hide resolved
client/src/advertisement/about/index.tsx Outdated Show resolved Hide resolved
client/src/plus/index.tsx Show resolved Hide resolved
Comment on lines 74 to 77
["updates", "Filter and sort updates"],
["collections", "Collections of articles"],
["offline", "MDN Offline"],
["ads-free", "Ads free", "new"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel like these should be objects 🤔

client/src/settings/manage.tsx Outdated Show resolved Hide resolved
client/src/setupProxy.js Show resolved Hide resolved
build/spas.ts Outdated Show resolved Hide resolved
client/src/advertisement/with_us/index.tsx Outdated Show resolved Hide resolved
client/src/advertisement/about/index.tsx Outdated Show resolved Hide resolved
client/src/advertisement/with_us/index.tsx Outdated Show resolved Hide resolved
client/src/advertisement/with_us/index.tsx Outdated Show resolved Hide resolved
client/src/document/organisms/toc/placement.tsx Outdated Show resolved Hide resolved
client/src/document/organisms/toc/placement.tsx Outdated Show resolved Hide resolved
client/src/placement-context.tsx Outdated Show resolved Hide resolved
client/src/setupProxy.js Show resolved Hide resolved
@fiji-flo fiji-flo changed the title Kevel 1 Experiment1 Feb 14, 2023
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Second round or review (A-Z)

client/src/advertising/with_us/index.tsx Outdated Show resolved Hide resolved
client/src/advertising/with_us/index.tsx Outdated Show resolved Hide resolved
pong: PlacementStatus,
observer: IntersectionObserver | null = null
) {
navigator?.sendBeacon?.(
Copy link
Contributor

Choose a reason for hiding this comment

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

We wanted to pull the check inside here:

Suggested change
navigator?.sendBeacon?.(
navigator && navigator.sendBeacon?.(

Copy link
Member

Choose a reason for hiding this comment

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

Does this make a difference?

client/src/document/organisms/toc/placement.tsx Outdated Show resolved Hide resolved
deployer/aws-lambda/kevel/index.js Show resolved Hide resolved
deployer/aws-lambda/kevel/index.js Outdated Show resolved Hide resolved
Comment on lines +55 to +56
const userAgentHeader = request.headers["user-agent"];
const userAgent = userAgentHeader ? userAgentHeader[0].value : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

We only use these in one branch.

deployer/aws-lambda/kevel/index.js Outdated Show resolved Hide resolved
Comment on lines +193 to +196
return {
status: 405,
statusDescription: "METHOD_NOT_ALLOWED",
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd almost extract these into const HTTP_405_METHOD_NOT_ALLOWED for reuse.

client/src/settings/manage.tsx Outdated Show resolved Hide resolved
client/src/settings/manage.tsx Outdated Show resolved Hide resolved
deployer/aws-lambda/kevel/index.js Show resolved Hide resolved
deployer/aws-lambda/kevel/index.js Outdated Show resolved Hide resolved
}
} else {
payload = {
copy: contents?.[0]?.data?.title || "🦖",
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps just make this optional in the type in placement.tsx, and deal with a null value there?

deployer/aws-lambda/kevel/index.js Outdated Show resolved Hide resolved
deployer/aws-lambda/kevel/index.js Show resolved Hide resolved
deployer/aws-lambda/kevel/index.js Outdated Show resolved Hide resolved
deployer/aws-lambda/kevel/index.js Show resolved Hide resolved
Comment on lines +175 to +195
function getIsDocumentHidden() {
if (typeof document !== "undefined") {
return !document.hidden;
}
return false;
}

export function usePageVisibility() {
const [isVisible, setIsVisible] = React.useState(getIsDocumentHidden());
const onVisibilityChange = () => setIsVisible(getIsDocumentHidden());
useEffect(() => {
if (typeof document !== "undefined") {
const visibilityChange = "visibilitychange";
document.addEventListener(visibilityChange, onVisibilityChange, false);
return () => {
document.removeEventListener(visibilityChange, onVisibilityChange);
};
}
});
return isVisible;
}
Copy link
Member

Choose a reason for hiding this comment

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

This whole section of code is a bit weird, how about:

Suggested change
function getIsDocumentHidden() {
if (typeof document !== "undefined") {
return !document.hidden;
}
return false;
}
export function usePageVisibility() {
const [isVisible, setIsVisible] = React.useState(getIsDocumentHidden());
const onVisibilityChange = () => setIsVisible(getIsDocumentHidden());
useEffect(() => {
if (typeof document !== "undefined") {
const visibilityChange = "visibilitychange";
document.addEventListener(visibilityChange, onVisibilityChange, false);
return () => {
document.removeEventListener(visibilityChange, onVisibilityChange);
};
}
});
return isVisible;
}
export function usePageVisibility() {
const [isVisible, setIsVisible] = React.useState(
typeof document === "undefined"
? false
: document.visibilityState === "visible"
);
useEffect(() => {
const onVisibilityChange = () =>
setIsVisible(document.visibilityState === "visible");
document.addEventListener("visibilitychange", onVisibilityChange);
return () => {
document.removeEventListener("visibilitychange", onVisibilityChange);
};
}, []);
return isVisible;
}
}

@fiji-flo fiji-flo changed the title Experiment1 feat: add placements Feb 15, 2023
@fiji-flo fiji-flo merged commit 3adeb45 into mdn:main Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file deployer Deployment (currently using AWS S3 and AWS Lambda) github-actions plus work around features related to MDN Plus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants