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

fix: enable ids in list of features from landing page #1087

Merged
merged 4 commits into from
Nov 9, 2022

Conversation

derberg
Copy link
Member

@derberg derberg commented Nov 8, 2022

Ids are critical for Google Tag manager. They are used to track how often people click on certain links on landing pages. The purpose is to learn if these are even useful if people care and interact with them.

cc @alequetzalli this is a fix for the issue we discussed recently. So basically answer is: there is no global issue with website that id are disabled. It was just a problem with single component after brand migration. So you can easily use id in tag manager

@alequetzalli the problem is I have no idea how we can make sure ids are not removed by anyone in next PRs. Without automation it can happen in future that these are removed. The only option I see is making you codeowner for docs related code. But this is not best solution, just responsibility is delegated to you 😄
Else? we could introduce automated tests. So start website instance, and check if id are there. You would have a config file where you have to write what id you add, and where are they expected to be 🤔

@netlify
Copy link

netlify bot commented Nov 8, 2022

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 52f938f
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/636affff445cdc0008ce63ae
😎 Deploy Preview https://deploy-preview-1087--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 33
🟠 Accessibility 88
🟠 Best practices 83
🟢 SEO 100
🔴 PWA 30

Lighthouse ran on https://deploy-preview-1087--asyncapi-website.netlify.app/

@quetzalliwrites
Copy link
Member

quetzalliwrites commented Nov 9, 2022

Point 1
Question: I don't understand the following sentence 😂✌🏽 "So basically answer is: there is no global issue with website that id are disabled."

Point 2
Yes, go ahead and make me codeowner for docs related changes, even outside of md files. I honestly have been leaning toward asking for that anyways, so it will be useful either way. 🙌🏽 I am already honestly a core leader, if you will, in all Docs UX and DevEx changes, so this honestly just makes sense.

Point 3
I think we should use IDs and educate our community on why we need these. Adding an automation test that checks for removed IDs in PRs sounds logical to me.

@akshatnema
Copy link
Member

/rtm

@derberg
Copy link
Member Author

derberg commented Nov 9, 2022

@alequetzalli

Point 1
oh sorry, when we had a call last week I shared that it might be that id are blocked globally like ignored by next.js maybe. That was one of my worst suspicions. Luckily I was wrong.

Point 2

#1089

Point 3

#1090

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants