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

Move static content site commands to tags #1539

Merged
merged 4 commits into from
Dec 8, 2021

Conversation

Numerlor
Copy link
Contributor

Creates the resources, faq and tools tags from the corresponding site commands and the site tag from the home/about command. The rules command was moved to the Information cog, and site help was removed along with the cog.

closes: #1473

@Numerlor
Copy link
Contributor Author

I've only moved the content to the bodies of the tags so far, do we also want to have a "title" in the tags?

@Xithrius Xithrius added a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) p: 3 - low Low Priority t: feature New feature or request a: tags Related to bot tags labels Apr 22, 2021
@ToxicKidz
Copy link
Contributor

do we also want to have a "title" in the tags?

How would this work? Currently we don't have embed titles for tags, unless you mean we should make the start of the files **title**, which I think would be a good idea.

@jb3
Copy link
Member

jb3 commented Apr 26, 2021

We could use a similar approach to the branding repo and put the tag metadata in the YAML frontmatter of the markdown documents.

For example:

---
title: How to get rich, fast.
---
1. Buy stonks
2. Watch stonks
3. ????
4. Profit

@Numerlor
Copy link
Contributor Author

We could use a similar approach to the branding repo and put the tag metadata in the YAML frontmatter of the markdown documents.

For example:

---
title: How to get rich, fast.
---
1. Buy stonks
2. Watch stonks
3. ????
4. Profit

I was only planning to do it with a bold text at the start like in ToxicKidz' comment, but implementing this shouldn't be too hard and will give more flexibility around tags in general so I'll do that.
Should the URLs also be moved to the title? I think it is a bit clearer inline but it may be worth it to have them in both places

@jb3
Copy link
Member

jb3 commented Apr 26, 2021

I was only planning to do it with a bold text at the start like in ToxicKidz' comment, but implementing this shouldn't be too hard and will give more flexibility around tags in general so I'll do that.

Thinking about it, that is likely out of scope for this specific PR. Maybe we group that in with #1545 so we can have richer tag data.

Should the URLs also be moved to the title?

Up to you, I know that the URL embeds don't look too great, and I think that the inline hyperlinks looks fine.

@Numerlor Numerlor mentioned this pull request Apr 26, 2021
@Numerlor
Copy link
Contributor Author

I'll wait for the feature to go through then

@Xithrius Xithrius added the s: stalled Something is blocking further progress label May 16, 2021
Copy link
Contributor

@Akarys42 Akarys42 left a comment

Choose a reason for hiding this comment

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

LGTM

@Akarys42
Copy link
Contributor

I don't think we should wait for tag grouping before moving along with this. It wouldn't change anything about the current functionality, we can group them later on when we merge the other PR.

@Numerlor
Copy link
Contributor Author

I don't think we should wait for tag grouping before moving along with this. It wouldn't change anything about the current functionality, we can group them later on when we merge the other PR.

The PR is stalled because the group PR should also introduce the ability to pass in embed arguments through the tag file to be used as titles, after the issue is approved. I suppose it could also be done in a PR separate from the grouping functionality but I don't think it'd be worth complicating it just to merge this PR

Copy link
Contributor

@onerandomusername onerandomusername left a comment

Choose a reason for hiding this comment

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

Looks good, one detail.

Testing the current behavior with the soon-to-be-removed site command, and noticing, with buttons, being able to link tags and get a nice view like this but with tags would be awesome.

image

bot/exts/info/information.py Outdated Show resolved Hide resolved
@mbaruh mbaruh added s: needs review Author is waiting for someone to review and approve and removed s: stalled Something is blocking further progress labels Dec 8, 2021
@mbaruh
Copy link
Member

mbaruh commented Dec 8, 2021

@Numerlor since the tags PR is merged I removed the stalled label.

@Numerlor
Copy link
Contributor Author

Numerlor commented Dec 8, 2021

@Numerlor since the tags PR is merged I removed the stalled label.

should be good to go now

Copy link
Member

@ChrisLovering ChrisLovering left a comment

Choose a reason for hiding this comment

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

Tested and all works good, bar my one comment.

bot/exts/info/information.py Outdated Show resolved Hide resolved
Discord does validation on the embed url which may fail for valid
local urls
@ChrisLovering ChrisLovering merged commit eb9e312 into python-discord:main Dec 8, 2021
@Numerlor Numerlor deleted the site-tags branch December 8, 2021 15:40
@Xithrius Xithrius removed the s: needs review Author is waiting for someone to review and approve label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: tags Related to bot tags p: 3 - low Low Priority t: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move static content site commands to tags
8 participants