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

Delete solution badges #149

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

leemthompo
Copy link
Contributor

@leemthompo leemthompo commented Oct 28, 2024

Addresses #88
Closes #146

We don't have link checking on these badges and the URLs might change again in the future, silently failing, so best to just remove solution badges entirely.

It's less pretty but ¯\_(ツ)_/¯

Screenshot:

Screenshot 2024-10-29 at 12 03 14

@leemthompo leemthompo requested a review from a team as a code owner October 28, 2024 15:37
@leemthompo leemthompo self-assigned this Oct 28, 2024
@elasticdocs
Copy link
Collaborator

elasticdocs commented Oct 28, 2024

🚀 Built elastic-dot-co-docs-preview-docs successfully!

Issues? Visit #next-docs in Slack

@bmorelli25
Copy link
Member

It looks like the badges are still trying to link out. Possibly because of the template?

@leemthompo
Copy link
Contributor Author

Yeah need to remove linking entirely from badges methinks.

Related PR: https://github.com/elastic/docsmobile/pull/946

@timto-elastic
Copy link

Do we need badges at all?

@leemthompo
Copy link
Contributor Author

Do we need badges at all?

Good question, they're a little neater than plain text. But in terms of opportunity cost of our time here, @bmorelli25 if removing the logic from the docsmobile code is too heavy a lift, we could just find and replace all badges with plaintext I guess.

@bmorelli25
Copy link
Member

I agree they're neater than plaintext. With that being said, I looked at https://github.com/elastic/docsmobile/pull/946 and this seems like more work than it's worth, especially for a system that we're hoping to migrate away from soon.

Sorry to have you change this PR, but I think it makes more sense to do away with our use of these entirely. That has the added benefit of simplifying migration.

@leemthompo leemthompo changed the title Delete links from badges Delete badges Oct 29, 2024
@leemthompo leemthompo changed the title Delete badges Delete solution badges Oct 29, 2024
@leemthompo
Copy link
Contributor Author

I've removed 'em entirely and updated the PR description to represent this new tack

Copy link
Member

@bmorelli25 bmorelli25 left a comment

Choose a reason for hiding this comment

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

Preview LGTM!

@leemthompo
Copy link
Contributor Author

@bmorelli25 @colleenmcginnis I don't know if you need this PR merged in light of re-migration, so I'll let y'all merge if needed. :)

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.

[Serverless]: Serverless Developer Tools -> Links result in error
5 participants