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

Prevent unnecessary nesting of breakpoint nodes (fixes #132) #158

Merged
merged 6 commits into from Oct 26, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/Breakpoints.ts
Expand Up @@ -134,6 +134,39 @@ export class Breakpoints<BreakpointKey extends string> {
}) as BreakpointKey | undefined
}

public toVisibleAtBreakpointSet(breakpointProps: MediaBreakpointProps) {
breakpointProps = this._normalizeProps(breakpointProps)
if (breakpointProps.lessThan) {
const breakpointIndex = this.sortedBreakpoints.findIndex(
bp => bp === breakpointProps.lessThan
)
return this.sortedBreakpoints.slice(0, breakpointIndex)
} else if (breakpointProps.greaterThan) {
const breakpointIndex = this.sortedBreakpoints.findIndex(
bp => bp === breakpointProps.greaterThan
)
return this.sortedBreakpoints.slice(breakpointIndex + 1)
} else if (breakpointProps.greaterThanOrEqual) {
const breakpointIndex = this.sortedBreakpoints.findIndex(
bp => bp === breakpointProps.greaterThanOrEqual
)
return this.sortedBreakpoints.slice(breakpointIndex)
} else if (breakpointProps.between) {
const between = breakpointProps.between
const fromBreakpointIndex = this.sortedBreakpoints.findIndex(
bp => bp === between[0]
)
const toBreakpointIndex = this.sortedBreakpoints.findIndex(
bp => bp === between[1]
)
return this.sortedBreakpoints.slice(
fromBreakpointIndex,
toBreakpointIndex
)
}
return []
}

public toRuleSets(keys = Breakpoints.validKeys()) {
const selectedMediaQueries = keys.reduce(
(mediaQueries, query) => {
Expand Down
164 changes: 98 additions & 66 deletions src/Media.tsx
Expand Up @@ -325,6 +325,12 @@ export function createMedia<
>({})
MediaContext.displayName = "Media.Context"

const MediaParentContext = React.createContext<{
hasParentMedia: boolean
breakpointProps: MediaBreakpointProps<BreakpointKey>
}>({ hasParentMedia: false, breakpointProps: {} })
MediaContext.displayName = "MediaParent.Context"

const MediaContextProvider: React.SFC<
MediaContextProviderProps<BreakpointKey | Interaction>
> = ({ disableDynamicMediaQueries, onlyMatch, children }) => {
Expand Down Expand Up @@ -380,76 +386,102 @@ export function createMedia<
className: "",
}

static contextType = MediaParentContext

render() {
const props = this.props
const {
children,
className: passedClassName,
interaction,
...breakpointProps
} = props
return (
<MediaContext.Consumer>
{({ onlyMatch } = {}) => {
let className: string | null
const {
children,
className: passedClassName,
interaction,
...breakpointProps
} = props
if (props.interaction) {
className = createClassName("interaction", props.interaction)
} else {
if (props.at) {
const largestBreakpoint =
mediaQueries.breakpoints.largestBreakpoint
if (props.at === largestBreakpoint) {
// TODO: We should look into making React’s __DEV__ available
// and have webpack completely compile these away.
let ownerName = null
try {
const owner = (this as any)._reactInternalFiber._debugOwner
.type
ownerName = owner.displayName || owner.name
} catch (err) {
// no-op
}

console.warn(
"[@artsy/fresnel] " +
"`at` is being used with the largest breakpoint. " +
"Consider using `<Media greaterThanOrEqual=" +
`"${largestBreakpoint}">\` to account for future ` +
`breakpoint definitions outside of this range.${
ownerName
? ` It is being used in the ${ownerName} component.`
: ""
}`
)
}
}

const type = propKey(breakpointProps)
const breakpoint = breakpointProps[type]!
className = createClassName(type, breakpoint)
}

const renderChildren =
onlyMatch === undefined ||
mediaQueries.shouldRenderMediaQuery(
{ ...breakpointProps, interaction },
onlyMatch
)

if (props.children instanceof Function) {
return props.children(className, renderChildren)
} else {
return (
<div
className={`fresnel-container ${className} ${passedClassName}`}
suppressHydrationWarning={!renderChildren}
>
{renderChildren ? props.children : null}
</div>
)
}
<MediaParentContext.Consumer>
{mediaParentContext => {
return (
<MediaParentContext.Provider
value={{ hasParentMedia: true, breakpointProps }}
>
<MediaContext.Consumer>
{({ onlyMatch } = {}) => {
let className: string | null
if (props.interaction) {
className = createClassName(
"interaction",
props.interaction
)
} else {
if (props.at) {
const largestBreakpoint =
mediaQueries.breakpoints.largestBreakpoint
if (props.at === largestBreakpoint) {
// TODO: We should look into making React’s __DEV__ available
// and have webpack completely compile these away.
let ownerName = null
try {
const owner = (this as any)._reactInternalFiber
._debugOwner.type
ownerName = owner.displayName || owner.name
} catch (err) {
// no-op
}

console.warn(
"[@artsy/fresnel] " +
"`at` is being used with the largest breakpoint. " +
"Consider using `<Media greaterThanOrEqual=" +
`"${largestBreakpoint}">\` to account for future ` +
`breakpoint definitions outside of this range.${
ownerName
? ` It is being used in the ${ownerName} component.`
: ""
}`
)
}
}

const type = propKey(breakpointProps)
const breakpoint = breakpointProps[type]!
className = createClassName(type, breakpoint)
}

const doesMatchParent =
!mediaParentContext.hasParentMedia ||
intersection(
mediaQueries.breakpoints.toVisibleAtBreakpointSet(
mediaParentContext.breakpointProps
),
mediaQueries.breakpoints.toVisibleAtBreakpointSet(
breakpointProps
)
).length > 0
const renderChildren =
doesMatchParent &&
(onlyMatch === undefined ||
mediaQueries.shouldRenderMediaQuery(
{ ...breakpointProps, interaction },
onlyMatch
))

if (props.children instanceof Function) {
return props.children(className, renderChildren)
} else {
return (
<div
className={`fresnel-container ${className} ${passedClassName}`}
suppressHydrationWarning={!renderChildren}
>
{renderChildren ? props.children : null}
</div>
)
}
}}
</MediaContext.Consumer>
</MediaParentContext.Provider>
)
}}
</MediaContext.Consumer>
</MediaParentContext.Consumer>
)
}
}
Expand Down
43 changes: 43 additions & 0 deletions src/__test__/Breakpoints.test.tsx
@@ -0,0 +1,43 @@
import { Breakpoints } from "../Breakpoints"

const config = {
"extra-small": 0,
small: 768,
medium: 1024,
large: 1120,
}

const breakpoint = new Breakpoints(config)

describe("Breakpoints", () => {
describe.only("toVisibleAtBreakpointSet", () => {
kirkness marked this conversation as resolved.
Show resolved Hide resolved
it("returns correct values for greaterThan", () => {
const breakpoints = breakpoint.toVisibleAtBreakpointSet({
greaterThan: "small",
})
expect(breakpoints).toEqual(["medium", "large"])
})
it("returns correct values for greaterThanOrEqual", () => {
const breakpoints = breakpoint.toVisibleAtBreakpointSet({
greaterThanOrEqual: "small",
})
expect(breakpoints).toEqual(["small", "medium", "large"])
})
it("returns correct values for lessThan", () => {
const breakpoints = breakpoint.toVisibleAtBreakpointSet({
lessThan: "small",
})
expect(breakpoints).toEqual(["extra-small"])
})
it("returns correct values for at", () => {
const breakpoints = breakpoint.toVisibleAtBreakpointSet({ at: "small" })
expect(breakpoints).toEqual(["small"])
})
it("returns correct values for between", () => {
const breakpoints = breakpoint.toVisibleAtBreakpointSet({
between: ["extra-small", "medium"],
})
expect(breakpoints).toEqual(["extra-small", "small"])
})
})
})
40 changes: 40 additions & 0 deletions src/__test__/Media.test.tsx
Expand Up @@ -530,6 +530,46 @@ describe("Media", () => {
})
})

describe("prevent nested unnecessary renders", () => {
it("only renders one element when Media is nested within Media", () => {
const query = renderer.create(
<MediaContextProvider>
<Media at="extra-small">
<span className="extra-small" />
</Media>

<Media at="medium">
<Media at="extra-small">
<span className="extra-small" />
</Media>
<Media at="medium">
<span className="medium" />
</Media>
<Media at="large">
<span className="large" />
</Media>
</Media>

<Media at="large">
<span className="large" />
</Media>
</MediaContextProvider>
)

expect(
query.root.findAllByProps({ className: "extra-small" }, { deep: true })
.length
).toBe(1)
alloy marked this conversation as resolved.
Show resolved Hide resolved
expect(
query.root.findAllByProps({ className: "large" }, { deep: true }).length
).toBe(1)
alloy marked this conversation as resolved.
Show resolved Hide resolved
expect(
query.root.findAllByProps({ className: "medium" }, { deep: true })
.length
).toBe(1)
alloy marked this conversation as resolved.
Show resolved Hide resolved
})
})

// TODO: This actually doesn’t make sense, I think, because if the user
// decides to not use a provider they are opting for rendering all
// variants. We just need to make sure to document this well.
Expand Down