-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Components v2 #7487
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
Components v2 #7487
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following message is repeated really often:
> info
> To use this component, you need to send the [message flag](/docs/resources/message#message-object-message-flags) `1 << 15` (IS_COMPONENTS_V2) which can be activated on a per-message basis.
Might it be better to just specify a version number in the heading, linking to "this needs flag xy"?
Anyways, I really love how the docs turned out. Might've overseen something. Will look a bit later again.
The suggestions should catch most > info
parts and change them to :::info
as Justin mentioned.
Also discussion point: Putting IS_COMPONENTS_V2 into ` IS_COMPONENTS_V2
Since I was asked to review
| 13 | [File](/docs/components/reference#file) | Displays an attached file | Content | Message | | ||
| 14 | [Separator](/docs/components/reference#separator) | Component to add vertical padding between other components | Layout | Message | | ||
| 17 | [Container](/docs/components/reference#container) | Container that visually groups a set of components | Layout | Message | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we decided against including the content inventory type (it can be received) as reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to point out a few things I saw :)
Sorry, if these are overly nit-picky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, since it came up again on our lib server: It should be mentioned that components (namely text display) can mention users and roles.
07ceead
to
51517d4
Compare
f5c4486
to
17d4a35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"just let me sneak one more merge conflict in first"
dammit it too late |
Adding documentation, changelog, and links updates for components v2. The interactions.js library and samples using it are going to follow in another PR.