From 5ab0866f9c587875633109bc82e175e01e3a82ba Mon Sep 17 00:00:00 2001 From: Henry Date: Tue, 20 Oct 2020 16:01:58 +0100 Subject: [PATCH 1/5] feat: working deep nesting prevention with initial passing test --- src/Breakpoints.ts | 33 ++++++ src/Media.tsx | 164 ++++++++++++++++++------------ src/__test__/Breakpoints.test.tsx | 43 ++++++++ src/__test__/Media.test.tsx | 40 ++++++++ 4 files changed, 214 insertions(+), 66 deletions(-) create mode 100644 src/__test__/Breakpoints.test.tsx diff --git a/src/Breakpoints.ts b/src/Breakpoints.ts index 143fea3..fc47840 100644 --- a/src/Breakpoints.ts +++ b/src/Breakpoints.ts @@ -134,6 +134,39 @@ export class Breakpoints { }) 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) => { diff --git a/src/Media.tsx b/src/Media.tsx index f93e8d4..ae75251 100644 --- a/src/Media.tsx +++ b/src/Media.tsx @@ -325,6 +325,12 @@ export function createMedia< >({}) MediaContext.displayName = "Media.Context" + const MediaParentContext = React.createContext<{ + hasParentMedia: boolean + breakpointProps: MediaBreakpointProps + }>({ hasParentMedia: false, breakpointProps: {} }) + MediaContext.displayName = "MediaParent.Context" + const MediaContextProvider: React.SFC< MediaContextProviderProps > = ({ disableDynamicMediaQueries, onlyMatch, children }) => { @@ -380,76 +386,102 @@ export function createMedia< className: "", } + static contextType = MediaParentContext + render() { const props = this.props + const { + children, + className: passedClassName, + interaction, + ...breakpointProps + } = props return ( - - {({ 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 `\` 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 ( -
- {renderChildren ? props.children : null} -
- ) - } + + {mediaParentContext => { + return ( + + + {({ 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 `\` 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 ( +
+ {renderChildren ? props.children : null} +
+ ) + } + }} + + + ) }} - + ) } } diff --git a/src/__test__/Breakpoints.test.tsx b/src/__test__/Breakpoints.test.tsx new file mode 100644 index 0000000..c32591f --- /dev/null +++ b/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", () => { + 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"]) + }) + }) +}) diff --git a/src/__test__/Media.test.tsx b/src/__test__/Media.test.tsx index 0a1692c..24de25f 100644 --- a/src/__test__/Media.test.tsx +++ b/src/__test__/Media.test.tsx @@ -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( + + + + + + + + + + + + + + + + + + + + + + ) + + expect( + query.root.findAllByProps({ className: "extra-small" }, { deep: true }) + .length + ).toBe(1) + expect( + query.root.findAllByProps({ className: "large" }, { deep: true }).length + ).toBe(1) + expect( + query.root.findAllByProps({ className: "medium" }, { deep: true }) + .length + ).toBe(1) + }) + }) + // 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. From 4b8df6f8a37b442562bdf79165511e383990056f Mon Sep 17 00:00:00 2001 From: Henry Date: Wed, 21 Oct 2020 11:46:15 +0100 Subject: [PATCH 2/5] test: add more tests --- src/__test__/Media.test.tsx | 163 ++++++++++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/src/__test__/Media.test.tsx b/src/__test__/Media.test.tsx index 24de25f..a370e07 100644 --- a/src/__test__/Media.test.tsx +++ b/src/__test__/Media.test.tsx @@ -568,6 +568,169 @@ describe("Media", () => { .length ).toBe(1) }) + + it("renders no spans with deep nesting where parent has no intersection with children", () => { + const query = renderer.create( + + + + + + + + + + + + + + + + + + + + ) + + expect(query.root.findAllByType("span", { deep: true }).length).toBe(0) + }) + + it("renders multiple spans in path, without rendering spans that don't intersect", () => { + const query = renderer.create( + + + {/* Should render */} + + + {/* Should render */} + + + {/* Should render */} + + + + {/* Should NOT render */} + + + + + {/* Should NOT render */} + + + + + ) + + expect(query.root.findAllByType("span", { deep: true }).length).toBe(3) + }) + + it("renders correct Media when using greaterThan prop", () => { + const query = renderer.create( + + + + {/* Should NOT render */} + + + + {/* Should NOT render */} + + + + {/* Should render */} + + + + {/* Should render */} + + + + + ) + + expect(query.root.findAllByType("span", { deep: true }).length).toBe(2) + }) + + it("renders correct Media when using greaterThanOrEqual prop", () => { + const query = renderer.create( + + + + {/* Should NOT render */} + + + + {/* Should render */} + + + + {/* Should render */} + + + + {/* Should render */} + + + + + ) + + expect(query.root.findAllByType("span", { deep: true }).length).toBe(3) + }) + + it("renders correct Media when using lessThan prop", () => { + const query = renderer.create( + + + + {/* Should render */} + + + + {/* Should NOT render */} + + + + {/* Should NOT render */} + + + + {/* Should NOT render */} + + + + + ) + + expect(query.root.findAllByType("span", { deep: true }).length).toBe(1) + }) + + it("renders correct Media when using lessThan prop", () => { + const query = renderer.create( + + + + {/* Should NOT render */} + + + + {/* Should render */} + + + + {/* Should render */} + + + + {/* Should NOT render */} + + + + + ) + + expect(query.root.findAllByType("span", { deep: true }).length).toBe(2) + }) }) // TODO: This actually doesn’t make sense, I think, because if the user From 9ad1ccbd7299ccca16939d2d2a1140b27f0f5c4d Mon Sep 17 00:00:00 2001 From: Henry Date: Mon, 26 Oct 2020 11:39:54 +0000 Subject: [PATCH 3/5] test: fix test name --- src/__test__/Media.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/__test__/Media.test.tsx b/src/__test__/Media.test.tsx index a370e07..c17b7d2 100644 --- a/src/__test__/Media.test.tsx +++ b/src/__test__/Media.test.tsx @@ -705,7 +705,7 @@ describe("Media", () => { expect(query.root.findAllByType("span", { deep: true }).length).toBe(1) }) - it("renders correct Media when using lessThan prop", () => { + it("renders correct Media when using between prop", () => { const query = renderer.create( From 1b000fecf9d13de8eacb1872e2ad9b0edcb472d9 Mon Sep 17 00:00:00 2001 From: Henry Date: Mon, 26 Oct 2020 11:42:56 +0000 Subject: [PATCH 4/5] Update src/__test__/Breakpoints.test.tsx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Eloy Durán --- src/__test__/Breakpoints.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/__test__/Breakpoints.test.tsx b/src/__test__/Breakpoints.test.tsx index c32591f..fa26796 100644 --- a/src/__test__/Breakpoints.test.tsx +++ b/src/__test__/Breakpoints.test.tsx @@ -10,7 +10,7 @@ const config = { const breakpoint = new Breakpoints(config) describe("Breakpoints", () => { - describe.only("toVisibleAtBreakpointSet", () => { + describe("toVisibleAtBreakpointSet", () => { it("returns correct values for greaterThan", () => { const breakpoints = breakpoint.toVisibleAtBreakpointSet({ greaterThan: "small", From f0384a8da4cf2f8f4581a923ef4dd4062b5a4266 Mon Sep 17 00:00:00 2001 From: Henry Date: Mon, 26 Oct 2020 11:58:06 +0000 Subject: [PATCH 5/5] test: remove non-nested medias from test --- src/__test__/Media.test.tsx | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/__test__/Media.test.tsx b/src/__test__/Media.test.tsx index c17b7d2..bbef388 100644 --- a/src/__test__/Media.test.tsx +++ b/src/__test__/Media.test.tsx @@ -534,10 +534,6 @@ describe("Media", () => { it("only renders one element when Media is nested within Media", () => { const query = renderer.create( - - - - @@ -549,20 +545,16 @@ describe("Media", () => { - - - - ) expect( query.root.findAllByProps({ className: "extra-small" }, { deep: true }) .length - ).toBe(1) + ).toBe(0) expect( query.root.findAllByProps({ className: "large" }, { deep: true }).length - ).toBe(1) + ).toBe(0) expect( query.root.findAllByProps({ className: "medium" }, { deep: true }) .length