Tweaking logic for warning when using conversation APIs that are missing accessibility parameters #1479
Conversation
…ing accessibility parameters like `text` or `fallback`, updating the warnings posted to make clear `text` is recommended and `fallback` is legacy. Fixes #1476
filmaj
added
enhancement
| @@ -645,20 +645,24 @@ function warnIfFallbackIsMissing(method: string, logger: Logger, options?: WebAP | |||
| const isTargetMethod = targetMethods.includes(method); | |||
|
|
|||
| const missingAttachmentFallbackDetected = (args: WebAPICallOptions) => Array.isArray(args.attachments) && | |||
| args.attachments.some((attachment) => !attachment.fallback || attachment.fallback.trim() === 0); | |||
| args.attachments.some((attachment) => !attachment.fallback || attachment.fallback.trim() === ''); | |||
There was a problem hiding this comment.
trim() returns the new string contents, and JS is funky and weird so actually '' == 0 is true but '' === 0 is false. So we can either turn the strict equality operator into just a regular equality operator and this would work - but I feel like that operation does not clearly convey what we are testing, so I opted for this slight change.
| logger.warn(buildWarningMessage('fallback')); | ||
| } else { | ||
| logger.warn(buildWarningMessage('text')); | ||
| const buildMissingFallbackWarning = () => `Additionally, the attachment-level \`fallback\` argument is missing in the request payload for a ${method} call - ` + |
There was a problem hiding this comment.
Could just as easily turn these two build*Warning methods into a simple string - if a reviewer has a preference, let me know, happy to change it.
There was a problem hiding this comment.
Having these two warning messages look better than the current one to me 👍
|
To other reviewers: let me know what you think of this approach. If approved, I will copy the same approach over to the python and java SDKs. |
| logger.warn(buildWarningMessage('fallback')); | ||
| } else { | ||
| logger.warn(buildWarningMessage('text')); | ||
| const buildMissingFallbackWarning = () => `Additionally, the attachment-level \`fallback\` argument is missing in the request payload for a ${method} call - ` + |
There was a problem hiding this comment.
Having these two warning messages look better than the current one to me 👍
|
@srajiang If you don't have any concerns on this PR, approve and merge this PR in your business hours tomorrow. |
|
@seratch yes, will work on this now! |
It is recommended to always provide a top-level
textparameter for use in screen readers, etc. There is also a legacyfallbackfield at the attachment level.This PR tweaks the logic slightly when displaying warnings to the user related to requests missing these fields. It also makes clear that
textis the recommended field to always use.This PR fixes #1476.