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

315 - Add GA custom event for Dataset to DOM elements #322

Merged
merged 230 commits into from
Sep 16, 2024

Conversation

nozomione
Copy link
Member

@nozomione nozomione commented Jan 22, 2024

Issue Number

Closing #315

Purpose/Implementation Notes

  • Added custom events for dataset to DOM elements using the GA custom events script
  • Verified that custom events are firing correctly in the development environment

To see the dataset custom GA4 reports, please login to CCDL Analytics account, and navigate to the development property Refine.bio Web - Dev (issue #310).

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

…r' into nozomione/237-display-info-message-to-certain-organism
… into nozomione/250-correcrt-text-color-for-non-linked-info-message
…essage' into nozomione/251-reverse-advanced-options-icon
…methods to update the filter order query in useSearchManager hook
…adjust the toggleFilter method in useSearchManage, and fetch the previous filter items in fetchSearch based on the filter orders
…options to include path and adjust the codebase
…ao in radio label, and add formatNumber (forgot) for the dowbloadable samples count in experiments view tab
…page' into nozomione/261-fix-clear-term-button-on-search-box
… nozomione/262-fix-typo-in-rna-seq-sample-compendia
…o nozomione/249-align-show-label-in-experiments-view-tab
…justment to copies in the custom error components (match the current definebio)
…, and make minior adjustment in useSamplesTableManager
…or/*, and make its index to be the Error parent component and use it to render its custom error child components in _error
…page' into nozomione/261-fix-clear-term-button-on-search-box
…nozomione/315-add-ga4-custom-event-for-dataset
…n (remove usePageRendered hook , no longer used)
Base automatically changed from nozomione/303-implement-ga4-custom-events-script to dev September 9, 2024 23:13
@nozomione
Copy link
Member Author

This PR is ready for your review, thank you David!

const handleClick = (link) => {
const handleShare = () => {
openModal(id)
gtag.trackSharedDataset(dataset)
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to when the user actually copies the link in handleCopy.

@@ -10,18 +11,24 @@ import { InlineMessage } from 'components/shared/InlineMessage'
import { Modal } from 'components/shared/Modal'
import { TextInput } from 'components/shared/TextInput'

export const ShareDatasetButton = ({ datasetId }) => {
export const ShareDatasetButton = ({ dataset }) => {
const { openModal } = useModal()
const { setResponsive } = useResponsive()
const { startTimer, clearTimer } = useTimeoutInCallback(() => {
setIsCopied(false)
}, 3000)
const [isCopied, setIsCopied] = useState(false)
const [value, handdleCopy] = useCopyToClipboard(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [value, handdleCopy] = useCopyToClipboard(null)
const [value, copyText] = useCopyToClipboard(null)

and then update below

gtag.trackSharedDataset(dataset)
}

const handleCopy = (link) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const handleCopy = (link) => {
const onCopyClick = () => {

const id = `shareable-link_${datasetId}`
const shareableLink = `${getDomain()}/dataset/${datasetId}?ref=share`

const handleClick = (link) => {
const handleShare = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const handleShare = () => {
const onShareClick = () => {

@@ -75,7 +82,7 @@ export const ShareDatasetButton = ({ datasetId }) => {
label="Copy"
primary
responsive
onClick={() => handleClick(shareableLink)}
onClick={() => handleCopy(shareableLink)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onClick={() => handleCopy(shareableLink)}
onClick={onCopyClick}

Lets not wrap the function here to keep it clearer

@@ -46,6 +47,11 @@ export const StartProcessingForm = ({ dataset }) => {

const pathname = `/dataset/${response.id}`
push({ pathname }, pathname)

if (datasetId === dataset.id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need the check here.

export const AddRemainingDatasetButton = ({
dataToAdd,
samplesInDataset,
label = 'Add Remaining'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can clean up the label here. Check the others as well please.

@@ -13,13 +14,14 @@ import { ReceiveUpdatesCheckBox } from 'components/Download/StartProcessingForm/
import { TermsOfUseCheckBox } from 'components/Download/StartProcessingForm/TermsOfUseCheckBox'

export const DownloadNowModal = ({
accessionCode,
experiment, // For GA
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
experiment, // For GA
experiment,

…lean up unused labels and a comment, and dataset ID checks
@nozomione
Copy link
Member Author

I've applied the feedback and this PR is ready for your review. Thank you David!

Copy link
Contributor

@davidsmejia davidsmejia left a comment

Choose a reason for hiding this comment

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

One comment that I think might need further explanation but I don't think it needs to hold up this PR.

@@ -26,7 +25,7 @@ export const SearchCardAction = ({
? technology === rnaSeq
: technology.find((x) => x === rnaSeq)

if (!pageRendered) return null
if (!experiment) return null
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having trouble figuring out how this component could be rendered without being passed in an experiment?

Copy link
Member Author

@nozomione nozomione Sep 16, 2024

Choose a reason for hiding this comment

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

I’ve removed the experiment check from the SearchCardAction component, as it was a quick fix for this PR. The component now accepts the entire data structure (i.e., experiment) for GA4 events, and I've also updated the parent components to reflect this change (see at fc602f7).

I'll update the other SearchCard-related components to match this pattern in a separate issue (#363) 👍

Additionally, I'll file a new issue to remove the usage of usePageRendered (which was added as a workaround to prevent the hydration errors) as chatted in our 1:1.

Thank you, David!

…e for GA4 event, adjust other props accordingly(Other SearchCard-related components will be updated to match this pattern in a separate issue), remove the experiment check from SearchCardAction
@nozomione nozomione merged commit a089d2d into dev Sep 16, 2024
2 checks passed
@nozomione nozomione deleted the nozomione/315-add-ga4-custom-event-for-dataset branch September 16, 2024 22:42
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.

Add GA custom event for Dataset to DOM elements
2 participants