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

help: Do not allow autolinks for help center docs. #30453

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

shubham-padia
Copy link
Member

mdxjs does not allow autolinks.
See mdx-js/mdx#1049.
We also added a linter check for the same.
Preparatory commit for #30451.

Fixes:

Screenshots and screen captures:

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

mdxjs does not allow autolinks.
See mdx-js/mdx#1049.
We also added a linter check for the same.
Preparatory commit for zulip#30451.
@shubham-padia shubham-padia marked this pull request as ready for review June 17, 2024 15:41
"description": """Autolinks are not allowed in /help documentation due to the upcoming migration to mdx.
Use Linkified markdown URLs [url](url) instead.
See https://github.com/mdx-js/mdx/issues/1049 for more info.""",
"include_only": {"help/"},
Copy link
Member

Choose a reason for hiding this comment

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

This is OK, but I'd usually write this copy in a way that doesn't require a future update after we actually migrate to MDX, since it's a small extra TODO list item for that migration. I don't have an easy proposal for that purpose, so probably the TODO is fine.

@timabbott timabbott merged commit a98363f into zulip:main Jun 17, 2024
14 checks passed
@timabbott
Copy link
Member

Merged, thanks @shubham-padia!

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

Successfully merging this pull request may close these issues.

3 participants