diff --git a/.github/workflows/pr_checklists.yml b/.github/workflows/pr_checklists.yml new file mode 100644 index 00000000000..61202f745ae --- /dev/null +++ b/.github/workflows/pr_checklists.yml @@ -0,0 +1,188 @@ +name: Pull Request Checklists + +on: + pull_request: + types: [opened, synchronize, reopened] + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + checklist: + runs-on: ubuntu-latest + + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Get changed files + id: files + run: | + # Ensure command doesn't fail if diff returns nothing + set -o pipefail + FILES=$(git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.event.pull_request.head.sha }} || echo "") + if [[ -z "$FILES" ]]; then + echo "No files changed in this PR." + echo "[]" > changed_files.txt + else + echo "$FILES" > changed_files.txt + fi + + - name: Determine which checklists to include + id: checklists + run: | + INCLUDE_CSS=false + INCLUDE_ANALYTICS=false + INCLUDE_L10N=false + INCLUDE_EXPERIMENT=false + INCLUDE_HTML=false + INCLUDE_JS=false + + while IFS= read -r FILE; do + # Skip if file doesn't exist or is not readable + if [[ ! -f "$FILE" || ! -r "$FILE" ]]; then + echo "Skipping non-existing or unreadable file: $FILE" + continue + fi + + # Check file names first (simplest checks) + [[ "$INCLUDE_CSS" != "true" && "$FILE" == *.scss ]] && INCLUDE_CSS=true + [[ "$INCLUDE_L10N" != "true" && "$FILE" == *.ftl ]] && INCLUDE_L10N=true + [[ "$INCLUDE_HTML" != "true" && "$FILE" == *.html ]] && INCLUDE_HTML=true + [[ "$INCLUDE_JS" != "true" && "$FILE" == *.js ]] && INCLUDE_JS=true + [[ "$INCLUDE_EXPERIMENT" != "true" && "$FILE" == *experiment* ]] && INCLUDE_EXPERIMENT=true + + # Check HTML files for links and buttons + if [[ "$INCLUDE_ANALYTICS" != "true" && "$FILE" == *.html ]]; then + if grep -qE ']*href=|/dev/null; then + INCLUDE_ANALYTICS=true + fi + fi + + # Check JS files for event listeners and experiment views + if [[ "$FILE" == *.js ]]; then + if [[ "$INCLUDE_ANALYTICS" != "true" ]]; then + grep -qE '\.addEventListener|onclick|onchange|onsubmit' "$FILE" 2>/dev/null && INCLUDE_ANALYTICS=true + fi + if [[ "$INCLUDE_EXPERIMENT" != "true" ]]; then + grep -q 'experiment_view' "$FILE" 2>/dev/null && INCLUDE_EXPERIMENT=true + fi + fi + + # Check if any views.py files have values in the entrypoint_variation allow list + if [[ "$INCLUDE_EXPERIMENT" != "true" && "$FILE" == *views.py ]]; then + if grep -q 'entrypoint_variation' "$FILE" 2>/dev/null; then + VALUE=$(grep 'entrypoint_variation' "$FILE" 2>/dev/null | grep -v '\[\s*\]' || true) + [[ -n "$VALUE" ]] && INCLUDE_EXPERIMENT=true + fi + fi + + # Stop looping through files if everything is already included + if [[ "$INCLUDE_CSS" == "true" && "$INCLUDE_ANALYTICS" == "true" && + "$INCLUDE_L10N" == "true" && "$INCLUDE_EXPERIMENT" == "true" && + "$INCLUDE_HTML" == "true" && "$INCLUDE_JS" == "true" ]]; then + # All flags are set, no need to check more files + break + fi + + done < changed_files.txt + + # Store results for next steps + echo "css=$INCLUDE_CSS" >> $GITHUB_OUTPUT + echo "analytics=$INCLUDE_ANALYTICS" >> $GITHUB_OUTPUT + echo "l10n=$INCLUDE_L10N" >> $GITHUB_OUTPUT + echo "experiment=$INCLUDE_EXPERIMENT" >> $GITHUB_OUTPUT + echo "html=$INCLUDE_HTML" >> $GITHUB_OUTPUT + echo "js=$INCLUDE_JS" >> $GITHUB_OUTPUT + + - name: Build comment body + id: comment + run: | + # Check if checklist directory exists + if [[ ! -d "docs/docs/checklists" ]]; then + echo "Error: docs/docs/checklists directory not found!" >&2 + exit 1 + fi + + BODY="Thank you for your pull request. Based on the contents you may need to double-check these things:\n" + + # Flag to track if any checklist was added + CHECKLIST_ADDED=false + + append_checklist() { + local title="$1" + local file="$2" + echo -e "\n---\n\n
\n$title\n\n$(cat $file)\n
\n" >> comment.txt + CHECKLIST_ADDED=true + } + + echo -e "$BODY" > comment.txt + + [[ "${{ steps.checklists.outputs.css }}" == "true" ]] && append_checklist "๐Ÿงพ CSS Checklist" "docs/docs/checklists/css.md" + [[ "${{ steps.checklists.outputs.analytics }}" == "true" ]] && append_checklist "๐Ÿ“Š Analytics Checklist" "docs/docs/checklists/analytics.md" + [[ "${{ steps.checklists.outputs.l10n }}" == "true" ]] && append_checklist "๐ŸŒ L10n Checklist" "docs/docs/checklists/l10n.md" + [[ "${{ steps.checklists.outputs.experiment }}" == "true" ]] && append_checklist "๐Ÿงช Experiment Checklist" "docs/docs/checklists/experiment.md" + [[ "${{ steps.checklists.outputs.html }}" == "true" ]] && append_checklist "๐Ÿงฑ HTML Checklist" "docs/docs/checklists/html.md" + [[ "${{ steps.checklists.outputs.js }}" == "true" ]] && append_checklist "๐Ÿ“œ JavaScript Checklist" "docs/docs/checklists/js.md" + + # If no checklists were added, add a simple message + if [[ "$CHECKLIST_ADDED" == "false" ]]; then + echo -e "\nNo specific checklists needed for this PR." >> comment.txt + fi + + # Save the comment content to an output for the next step + echo "body<> $GITHUB_OUTPUT + cat comment.txt >> $GITHUB_OUTPUT + echo "EOF" >> $GITHUB_OUTPUT + + - name: Check for existing comment + id: find-comment + uses: actions/github-script@v7 + with: + script: | + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number + }); + + const botComment = comments.find(comment => { + return comment.user.login === 'github-actions[bot]' && + comment.body.includes('Based on the contents you may need to double-check these things'); + }); + + if (botComment) { + core.setOutput('comment-id', botComment.id); + core.setOutput('exists', 'true'); + } else { + core.setOutput('exists', 'false'); + } + + - name: Update existing comment + if: steps.find-comment.outputs.exists == 'true' + uses: actions/github-script@v7 + with: + script: | + github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: ${{ steps.find-comment.outputs.comment-id }}, + body: `${{ steps.comment.outputs.body }}` + }); + + - name: Create new comment + if: steps.find-comment.outputs.exists == 'false' + uses: actions/github-script@v7 + with: + script: | + try { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + body: `${{ steps.comment.outputs.body }}` + }); + } catch (error) { + core.warning(`Failed to create comment: ${error.message}`); + } diff --git a/docs/docs/attribution/0001-analytics.md b/docs/docs/attribution/0001-analytics.md index 5737c73d918..67120b7078a 100644 --- a/docs/docs/attribution/0001-analytics.md +++ b/docs/docs/attribution/0001-analytics.md @@ -65,18 +65,11 @@ CTA ("Call to Action") click is intented to track the one or two main actions th The attribute `data-cta-text` must be present to trigger the event. All links to accounts.mozilla.com must also use `data-cta-type`. -| Data Attribute | Expected Value | -| ------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `data-cta-text` * | Text or name of the link (e.g. `Sign Up`, `Start Here`, `Get Relay`, `See your report`, `Read the Manifesto`). | -| | | -| | - This does not need to exactly match the text displayed to the user | -| | - Defining this is useful to group the link clicks across locales | -| | - - this attribute is required | -| `data-cta-position` | Location of CTA on the page (e.g. `primary`, `secondary`, `banner`, `pricing`) | -| `data-cta-type` | fxa-servicename (e.g. `fxa-sync`, `fxa-monitor`, `fxa-vpn`, `monitor`, `relay`) | -| | | -| | - This is to group CTAs by their destination | -| | - Do not use this to identify the element (ie. link, button) | +| Data Attribute | Expected Value | +| -------------- | -------------- | +| `data-cta-text` * | Text or name of the link (e.g. `Sign Up`, `Start Here`, `Get Relay`, `See your report`, `Read the Manifesto`).
- This does not need to exactly match the text displayed to the user
- Defining this is useful to group the link clicks across locales
* This attribute is required | +| `data-cta-position` | Location of CTA on the page (e.g. `primary`, `secondary`, `banner`, `pricing`) | +| `data-cta-type` | fxa-servicename (e.g. `fxa-sync`, `fxa-monitor`, `fxa-vpn`, `monitor`, `relay`)
- This is to group CTAs by their destination
- Do not use this to identify the element (ie. link, button) | | `data-cta-name` | A identifier for this cta that is unique across the entire site. (e.g. `fx20-primarycta`, `wnp118-sfaq-so-special-features`). This is to help with reporting since it is difficult to filter on more than one parameter at a time. | ``` html @@ -93,10 +86,10 @@ Link click is intended to track links that are of interest but not the focus of The attribute `data-link-text` must be present to trigger the event. -| Data Attribute | Expected Value | -| -------------------- | ---------------------------------------------------------------------------------------------------------------------------- | +| Data Attribute | Expected Value | +| -------------- | -------------- | | `data-link-text` * | Text or name of the link (e.g. `Monitor`, `Features`, `Instagram (mozilla)`, `Mozilla VPN`). - * this attribute is required | -| `data-link-position` | Location of CTA on the page (e.g. `topnav`, `subnav`, `body`, `features`) | +| `data-link-position` | Location of CTA on the page (e.g. `topnav`, `subnav`, `body`, `features`) | ``` html

This is text with a simpleexample.

@@ -292,46 +285,40 @@ TrackProductDownload.sendEvent(customEventObject); We are using the custom event `widget_action` to track the behaviour of javascript widgets. -**How do you chose between \`\`widget_action\`\` and \`\`cta_click\`\`?** - -| widget_action | cta_click | -| ----------------------------------------------------------- | -------------------------------------------------------- | -| The action is specific or unique. | The action is \"click\". | -| | | -| *(Only the language switcher changes* *the page language.)* | | -| The user does not leave the page. | It sends the user somewhere else. | -| It requires Javascript to work. | No JS required. | -| It can perform several actions. | It does one action. | -| | | -| *(A modal can be opened and closed.)* | | -| There could be several on the page doing different things. | There could be several on the page doing the same thing. | -| | | -| *(An accordion list of FAQs)* | *(A download button in the header and footer.)* | +**How do you chose between `widget_action` and `cta_click`?** + +| widget_action | cta_click | +| ------------- | --------- | +| The action is specific or unique.
*(example: only the language switcher changes the page language.)* | The action is "click". | +| The user does not leave the page. | It sends the user somewhere else. | +| It requires Javascript to work. | No JS required. | +| It can perform several actions.
*(example: modal can be opened and closed.)* | It does one action.| +| There could be several on the page doing different things.
*(An accordion list of FAQs)* | There could be several on the page doing the same thing.
*(A download button in the header and footer.)*| Properties for use with ``widget_action`` (not all widgets will use all options): - type - **Required.** - The type of widget. - - Examples: \"modal\", \"protection report\", \"affiliate notification\", \"help icon\". - - *Avoid "button" or "link". If you want to track a link or button use \`cta_click\`.* + - Examples: "modal", "protection report", "affiliate notification", "help icon". + - *Avoid "button" or "link". If you want to track a link or button use `cta_click`.* - action - **Required.** - The thing that happened. - - Examples: \"open\", \"accept\", \"timeout\", \"vote up\". + - Examples: "open", "accept", "timeout", "vote up". - *Avoid "click". If you want to track a click use \`cta_click\`.* - text - How is this action labeled to the user? - - Examples: \"Okay\", \"Check your protection report\", \"Get the app\" + - Examples: "Okay", "Check your protection report", "Get the app" - name - Give the widget a name. - - You probably only need this optional attribute if the ``text`` value is not enough to tell the widgets apart. + - You probably only need this optional attribute if the `text` value is not enough to tell the widgets apart. - This can help you group actions from the same widget, or make it easier to find the widget in the reports. - The dashes are not required but they\'re allowed if you want to match the element class or ID. - - Examples: \"dad-joke-banner\", \"focus-qr-code\", \"Join Firefox Modal\" + - Examples: "dad-joke-banner", "focus-qr-code", "Join Firefox Modal" - non_interaction (boolean) - True if the action was triggered by something other than a user gesture. diff --git a/docs/docs/checklists/analytics.md b/docs/docs/checklists/analytics.md new file mode 100644 index 00000000000..815cf614413 --- /dev/null +++ b/docs/docs/checklists/analytics.md @@ -0,0 +1,39 @@ +## Links `` + +- [ ] Has a `data-cta-text` OR `data-link-text` + - [ ] If it has class `mzp-c-button` it is a CTA + - [ ] If it has class `mzp-c-cta-link` it is a CTA + - [ ] If there are two of the same `data-cta-text` include `data-cta-position` + - [ ] Does not have both `data-cta-text` and `data-link-text` +- [ ] If linking to another Mozilla property include `utm` params + - [ ] `utm_source` is `'www.firefox.com` or `www.mozilla.org` + - [ ] `utm_medium` is `referral` + - [ ] if the query string is in a variable it is called `params` and the string includes the `?` +- Download button: + - [ ] Use the appropriate helper, don't hard code these. (`download_firefox_thanks`, `google_play_button`, `apple_app_store_button`) + - [ ] include a download_location if there are multiple buttons on the page + +# Buttons `