-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[docs-infra] Move Ads component to TypeScript #42842
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5,19 +5,17 @@ import Box from '@mui/material/Box'; | |||||
import Paper from '@mui/material/Paper'; | ||||||
import AdCarbon from 'docs/src/modules/components/AdCarbon'; | ||||||
import AdInHouse from 'docs/src/modules/components/AdInHouse'; | ||||||
import { GA_ADS_DISPLAY_RATIO } from 'docs/src/modules/constants'; | ||||||
import { AdContext, adShape } from 'docs/src/modules/components/AdManager'; | ||||||
import { useTranslate } from '@mui/docs/i18n'; | ||||||
|
||||||
function PleaseDisableAdblock(props) { | ||||||
function PleaseDisableAdblock() { | ||||||
const t = useTranslate(); | ||||||
|
||||||
return ( | ||||||
<Paper | ||||||
component="span" | ||||||
elevation={0} | ||||||
sx={{ display: 'block', p: 1.5, border: '2px solid', borderColor: 'primary.main' }} | ||||||
{...props} | ||||||
> | ||||||
<Typography variant="body2" component="span" gutterBottom sx={{ display: 'block' }}> | ||||||
{t('likeMui')} | ||||||
|
@@ -74,7 +72,10 @@ const inHouseAds = [ | |||||
}, | ||||||
]; | ||||||
|
||||||
class AdErrorBoundary extends React.Component { | ||||||
class AdErrorBoundary extends React.Component<{ | ||||||
eventLabel: string | null; | ||||||
children?: React.ReactNode | undefined; | ||||||
}> { | ||||||
static propTypes = { | ||||||
children: PropTypes.node.isRequired, | ||||||
eventLabel: PropTypes.string, | ||||||
|
@@ -120,8 +121,8 @@ function isBot() { | |||||
} | ||||||
|
||||||
export default function Ad() { | ||||||
const [adblock, setAdblock] = React.useState(null); | ||||||
const [carbonOut, setCarbonOut] = React.useState(null); | ||||||
const [adblock, setAdblock] = React.useState<null | boolean>(null); | ||||||
const [carbonOut, setCarbonOut] = React.useState<null | boolean>(null); | ||||||
|
||||||
const { current: randomAdblock } = React.useRef(Math.random()); | ||||||
const { current: randomInHouse } = React.useRef(Math.random()); | ||||||
|
@@ -150,7 +151,7 @@ export default function Ad() { | |||||
const ad = React.useContext(AdContext); | ||||||
const eventLabel = label ? `${label}-${ad.placement}-${adShape}` : null; | ||||||
|
||||||
const timerAdblock = React.useRef(); | ||||||
const timerAdblock = React.useRef<NodeJS.Timeout>(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The codebase uses:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Modification done in #42944 |
||||||
|
||||||
const checkAdblock = React.useCallback( | ||||||
(attempt = 1) => { | ||||||
|
@@ -162,7 +163,7 @@ export default function Ad() { | |||||
) { | ||||||
if ( | ||||||
document.querySelector('#carbonads a') && | ||||||
document.querySelector('#carbonads a').getAttribute('href') === | ||||||
document.querySelector('#carbonads a')?.getAttribute('href') === | ||||||
'https://material-ui-next.com/discover-more/backers' | ||||||
) { | ||||||
setCarbonOut(true); | ||||||
|
@@ -198,7 +199,7 @@ export default function Ad() { | |||||
|
||||||
React.useEffect(() => { | ||||||
// Avoid an exceed on the Google Analytics quotas. | ||||||
if (Math.random() > GA_ADS_DISPLAY_RATIO || !eventLabel) { | ||||||
if (Math.random() > ((process.env.GA_ADS_DISPLAY_RATIO ?? 0.1) as number) || !eventLabel) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing to note as well is that Next.js actually puts a number in |
||||||
return undefined; | ||||||
} | ||||||
|
||||||
|
@@ -231,10 +232,6 @@ export default function Ad() { | |||||
display: 'flex', | ||||||
alignItems: 'flex-end', | ||||||
}), | ||||||
...(adShape === 'inline2' && { | ||||||
display: 'flex', | ||||||
alignItems: 'flex-end', | ||||||
}), | ||||||
})} | ||||||
Comment on lines
234
to
235
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not found usage of |
||||||
data-ga-event-category="ad" | ||||||
data-ga-event-action="click" | ||||||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
import * as React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import { unstable_useEnhancedEffect as useEnhancedEffect } from '@mui/material/utils'; | ||
|
||
type AdPortal = { | ||
placement: 'body-top'; | ||
element: Element | null; | ||
}; | ||
|
||
interface AdManagerProps { | ||
/** | ||
* The querySelector use to target the element which will include the ad. | ||
*/ | ||
classSelector?: string; | ||
children?: React.ReactNode | undefined; | ||
} | ||
|
||
export const AdContext = React.createContext<AdPortal>({ placement: 'body-top', element: null }); | ||
|
||
// Persisted for the whole session. | ||
// The state is used to use different ad placements. | ||
const randomSession = Math.random(); | ||
|
||
// Distribution profile: | ||
// 20% body-inline | ||
// 80% body-image | ||
export const adShape = randomSession < 0.2 ? 'inline' : 'image'; | ||
|
||
export default function AdManager({ classSelector = '.description', children }: AdManagerProps) { | ||
const [portal, setPortal] = React.useState<AdPortal>({ placement: 'body-top', element: null }); | ||
|
||
useEnhancedEffect(() => { | ||
const container = document.querySelector(classSelector); | ||
setPortal({ placement: 'body-top', element: container }); | ||
}, [classSelector]); | ||
|
||
return <AdContext.Provider value={portal}>{children}</AdContext.Provider>; | ||
} | ||
|
||
AdManager.propTypes = { | ||
children: PropTypes.node, | ||
classSelector: PropTypes.string, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import Gtag from '@types/gtag.js'; | ||
|
||
declare global { | ||
interface Window { | ||
gtag: Gtag.Gtag; | ||
} | ||
} |
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.
Not sure it's the best place to move the constants
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.
imo, ideally we move
_app
to the docs infra package, add the ads display ratio as a prop and provide it in a context.For now I guess the approach works, though I don't like to depend on
process.env
in libraries, it's implicit API and bundlers may not always handle it.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.
Can we revert this?
Also related to vercel/next.js#57755 (comment)