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

Update NSO and inquiry FAQ link on artwork sidebar (PURCHASE-1885) #5721

Merged
merged 3 commits into from
Jun 9, 2020

Conversation

starsirius
Copy link
Member

.find('a[children="Conditions of Sale"]')
.at(0)
.simulate("click")
wrapper.find('a[children="Conditions of Sale"]').at(0).simulate("click")
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the large diff here. Looks like there was some prettier update that caused the format changes. Probably safe to ignore.

@@ -137,37 +125,33 @@ describe("ArtworkSidebarExtraLinks", () => {
})
it("displays proper text", () => {
expect(wrapper.text()).toContain(
"Have a question? Read our FAQ or ask a specialist."
"Have a question? Visit our help center or ask a specialist."
Copy link
Member Author

Choose a reason for hiding this comment

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

These are real changes.

@@ -150,7 +154,9 @@ ArtworkSidebarExtraLinksContainerProps
return (
<Container>
Have a question?{" "}
<Link onClick={this.onClickBuyNowFAQ.bind(this)}>Read our FAQ</Link>{" "}
<Link onClick={this.onClickBuyNowFAQ.bind(this)}>
Visit our help center
Copy link
Member Author

Choose a reason for hiding this comment

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

Noticed that we track clicks for Buy Now FAQ link and Collector FAQ link below. I assume the CTA copy changes may effect how users interact with the page. Do we want to modify the tracking or are we fine keeping the existing tracking as is? cc @anipetrov

Copy link
Contributor

Choose a reason for hiding this comment

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

let's change the label property to the new CTA please!

Copy link
Contributor

@zephraph zephraph left a comment

Choose a reason for hiding this comment

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

👍 Once your question about tracking is answered feel free to merge.

@starsirius
Copy link
Member Author

Merging!

@starsirius starsirius merged commit a2c621c into artsy:master Jun 9, 2020
@artsy-peril artsy-peril bot mentioned this pull request Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants