-
Notifications
You must be signed in to change notification settings - Fork 218
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
Add RichTextBlock support to Block Kit Kotlin DSL builder #1376
Conversation
Thanks for the contribution! Before we can merge this, we need @KENNYSOFT to sign the Salesforce Inc. Contributor License Agreement. |
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.
Great work! Overall, your changes look great to me but could you take a bit more time to adjust the following?
- A few more adjustments to your changes; see my review comments
- Run
./scripts/run_no_prep_tests.sh
to generate corresponding JSON files - Sign our CLA; without it, we are unable to accept any external contribution
Thank you!
slack-api-client/src/main/java/com/slack/api/util/json/GsonFactory.java
Outdated
Show resolved
Hide resolved
...-extension/src/main/kotlin/com/slack/api/model/kotlin_extension/block/element/ButtonStyle.kt
Outdated
Show resolved
Hide resolved
.../com/slack/api/model/kotlin_extension/block/element/container/SingleBlockElementContainer.kt
Outdated
Show resolved
Hide resolved
* @param name The name of the emoji; i.e. "wave" or "wave::skin-tone-2". | ||
* @see <a href="https://api.slack.com/reference/block-kit/blocks#emoji-element-type">Rich text object documentation</a> | ||
*/ | ||
fun emoji(name: String) |
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.
emoji method should accept two patterns:
- name: String, skinTone: Int? = null, style: TextStyle? = null
- unicode: String
overloading with string type may not work; if that's the case, skipping unicode here is okay
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.
https://api.slack.com/reference/block-kit/blocks#emoji-element-type
I've implemented based on the documents, but yes I've seen that RichTextSectionElement.Emoji
has those fields and will take a look for them.
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.
Resolved in 1be897c
By the way, Kotlin allows for overloading but I think it can be confusing. With this interface:
fun emoji(name: String, skinTone: Int? = null, style: LimitedTextStyle? = null)
fun emoji(unicode: String)
The call emoji("aaa")
is directed to the second one, and to use the first method then we should use a named parameter like emoji(name = "aaa")
.
So I've skipped the Unicode version for now, but also suggesting a different DSL like emojiUnicode("U+AC00")
with little confidence.
.../main/kotlin/com/slack/api/model/kotlin_extension/block/composition/dsl/RichTextObjectDsl.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/slack/api/model/kotlin_extension/block/composition/dsl/RichTextObjectDsl.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/slack/api/model/kotlin_extension/block/composition/dsl/RichTextObjectDsl.kt
Outdated
Show resolved
Hide resolved
slack-api-model/src/main/java/com/slack/api/model/block/element/RichTextQuoteElement.java
Show resolved
Hide resolved
This reverts commit a892eb8.
This comment was marked as resolved.
This comment was marked as resolved.
Thanks! |
As a workaround, I closed and reopend this PR to let the CLA bot check it again. It seems your status is now correctly updated! |
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.
Looks good to me! Once the CI builds pass, I will merge this PR. Thank you so much for the contribution!
Oops, let me correct myself about the merge timing, we'll release this patch release before it: https://github.com/slackapi/java-slack-sdk/milestone/108 After that, this enhancement will be included in an upcoming minor release. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1376 +/- ##
============================================
+ Coverage 74.24% 74.49% +0.24%
- Complexity 4273 4324 +51
============================================
Files 461 472 +11
Lines 13364 13491 +127
Branches 1377 1377
============================================
+ Hits 9922 10050 +128
+ Misses 2629 2628 -1
Partials 813 813 ☔ View full report in Codecov by Sentry. |
c4fd8ae
to
12c2ea7
Compare
This PR adds RichTextBlock support to Block Kit Kotlin DSL builder. With this changes, we can now use DSL like:
Category (place an
x
in each of the[ ]
)Requirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.