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

Simplify A/B test lookup via express-http-context #3191

Merged
merged 10 commits into from
Feb 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions .babelrc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
}
],
"@babel/plugin-proposal-class-properties",
"@babel/plugin-proposal-optional-chaining",
Copy link
Member Author

Choose a reason for hiding this comment

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

Suprised this wasn't already in! We can no do

const deepPropOrUndefined = foo?.bar?.baz?.bam

which should eliminate our need for the get util.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oo sick!

"@babel/plugin-proposal-nullish-coalescing-operator",
[
"relay",
{
Expand Down
4 changes: 4 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"peerDependencies": {
"@artsy/palette": "^4.16.0 || ^5.0.0 || ^7.0.0",
"@sentry/browser": "^4.2.3",
"express-http-context": "^1.2.3",
"flickity": "2.1.2",
"react": "^16.8.0",
"react-dom": "^16.8.0",
Expand Down Expand Up @@ -57,6 +58,8 @@
"@babel/plugin-proposal-class-properties": "7.3.4",
"@babel/plugin-proposal-decorators": "7.3.0",
"@babel/plugin-proposal-json-strings": "7.2.0",
"@babel/plugin-proposal-nullish-coalescing-operator": "^7.8.3",
"@babel/plugin-proposal-optional-chaining": "^7.8.3",
"@babel/plugin-syntax-dynamic-import": "7.8.3",
"@babel/plugin-syntax-import-meta": "7.0.0",
"@babel/preset-env": "7.3.4",
Expand Down Expand Up @@ -115,6 +118,7 @@
"dotenv": "4.0.0",
"enzyme": "3.8.0",
"enzyme-adapter-react-16": "1.7.1",
"express-http-context": "^1.2.3",
"flickity": "2.1.2",
"fork-ts-checker-notifier-webpack-plugin": "0.6.2",
"fork-ts-checker-webpack-plugin": "0.4.10",
Expand Down
19 changes: 16 additions & 3 deletions src/Apps/Artwork/ArtworkApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export interface Props {
artwork: ArtworkApp_artwork
tracking?: TrackingProp
routerPathname: string
referrer: string
}

declare const window: any
Expand All @@ -53,6 +54,7 @@ export class ArtworkApp extends React.Component<Props> {
trackPageview() {
const {
artwork: { listPrice, availability, is_offerable, is_acquireable },
referrer,
} = this.props

// FIXME: This breaks our global pageview tracking in the router level.
Expand All @@ -64,6 +66,7 @@ export class ArtworkApp extends React.Component<Props> {
offerable: is_offerable,
availability,
price_listed: !!listPrice,
referrer,
}

if (typeof window.analytics !== "undefined") {
Expand Down Expand Up @@ -222,7 +225,14 @@ export class ArtworkApp extends React.Component<Props> {
</Col>
</Row>

<div id="lightbox-container" />
<div
id="lightbox-container"
style={{
position: "absolute",
top: 0,
zIndex: 1100, // over top nav
}}
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes long standing issue where when a user views an artwork in full screen, the top nav bar flops out due to z-index and position.

/>
<SystemContextConsumer>
{({ mediator }) => <>{this.enableIntercomForBuyers(mediator)}</>}
</SystemContextConsumer>
Expand All @@ -236,11 +246,14 @@ export const ArtworkAppFragmentContainer = createFragmentContainer(
(props: Props) => {
const {
match: {
location: { pathname },
location: { pathname, state },
},
} = useContext(RouterContext)

return <ArtworkApp {...props} routerPathname={pathname} />
const referrer = state && state.previousHref
return (
<ArtworkApp {...props} routerPathname={pathname} referrer={referrer} />
)
},
{
artwork: graphql`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export class ArtworkActions extends React.Component<
if (is_downloadable || this.isAdmin) {
const artistNames = artists.map(({ name }) => name).join(", ")
const filename = slugify(compact([artistNames, title, date]).join(" "))
const downloadableImageUrl = `${sd.APP_URL}${href}/download/${filename}.jpg` // prettier-ignore
const downloadableImageUrl = `${href}/download/${filename}.jpg` // prettier-ignore
return downloadableImageUrl
}
}
Expand Down
14 changes: 6 additions & 8 deletions src/Apps/Artwork/Components/OtherWorks/Header.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Button, Flex, Serif } from "@artsy/palette"
import { RouterLink } from "Artsy/Router/RouterLink"
import React from "react"

interface HeaderProps {
Expand All @@ -16,14 +17,11 @@ export const Header: React.SFC<HeaderProps> = props => {
{title}
</Serif>
{buttonHref && (
<Button
variant="secondaryOutline"
mb={3}
// FIXME: Move to <Link> component once https://github.com/artsy/palette/pull/130 is merged
onClick={() => (window.location.href = buttonHref)}
>
View all
</Button>
<RouterLink to={buttonHref}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah cool, yea these custom window.location.href navigations in JS dont work with our router.

<Button variant="secondaryOutline" mb={3}>
View all
</Button>
</RouterLink>
)}
{children}
</Flex>
Expand Down
2 changes: 1 addition & 1 deletion src/Artsy/Analytics/trackingMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export function trackingMiddleware(options: TrackingMiddlewareOptions = {}) {
}

// TODO: Remove after EXPERIMENTAL_APP_SHELL AB test ends.
if (getENV("EXPERIMENTAL_APP_SHELL")) {
if (sd.CLIENT_NAVIGATION_V3) {
trackExperimentViewed("client_navigation_v3")
}

Expand Down
2 changes: 1 addition & 1 deletion src/Artsy/Relay/renderWithLoadProgress.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export function renderWithLoadProgress<P>(
initialProps: object = {},
wrapperProps: object = {},
spinnerProps: SpinnerProps = {
delay: 800,
delay: 1000,
}
): LoadProgressRenderer<P> {
// TODO: We need design for retrying or the approval to use the iOS design.
Expand Down
10 changes: 9 additions & 1 deletion src/Artsy/Router/makeAppRoutes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,15 @@ export function makeAppRoutes(routeList: RouteList[]): RouteConfig[] {
const foundUrl = props.router.matcher.matchRoutes(routes, url)

if (foundUrl) {
props.router.push(url)
const location = props.router.createLocation(url)
const previousHref = window.location.href

props.router.push({
...location,
state: {
previousHref,
},
})
} else {
window.location.assign(url)
}
Expand Down
10 changes: 9 additions & 1 deletion src/Utils/getENV.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@ import { data as sd } from "sharify"

export function getENV(ENV_VAR) {
const isServer = typeof window === "undefined"
const envVar = isServer ? process.env[ENV_VAR] : sd[ENV_VAR]

let envVar
if (isServer) {
const httpContext = require("express-http-context")
envVar = httpContext.get(ENV_VAR) ?? process.env[ENV_VAR]
Copy link
Member Author

Choose a reason for hiding this comment

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

Uses new nullish coalesing plugin, which always returns the left hand side of the operation unless null or undefined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting. I didn't fully get immediately how different is than ||. I do now recognize the subtle difference, but I think generally any sort of conditional assignment like this, if you are expecting a false-y/null/undefined to be possible (or even valid, esp. as a default), I almost always open a console to try it out (and/or mentally unpack it in my brain for a second).

Cool! I look forward to trying this.

} else {
envVar = sd[ENV_VAR]
}

return envVar
}
1 change: 1 addition & 0 deletions typings/sharify.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ declare module "sharify" {
readonly APP_URL: string
readonly ARTIST_COLLECTIONS_RAIL?: string // TODO: remove after CollectionsRail a/b test
readonly ARTIST_COLLECTIONS_RAIL_IDS: string[]
readonly CLIENT_NAVIGATION_V3: "experiment" | "control" // TODO: Remove after A/B test.
readonly CMS_URL: string
readonly ENABLE_PRICE_TRANSPARENCY: string
readonly ENABLE_REQUEST_CONDITION_REPORT: string
Expand Down
Loading