-
Notifications
You must be signed in to change notification settings - Fork 21
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 autocomplete suggestions #40
Conversation
Codecov Report
@@ Coverage Diff @@
## master #40 +/- ##
==========================================
- Coverage 21.03% 18.42% -2.61%
==========================================
Files 6 6
Lines 233 266 +33
==========================================
Hits 49 49
- Misses 173 206 +33
Partials 11 11
Continue to review full report at Codecov.
|
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.
@iomodo, this is awesome and I can't wait to add it to other plugins! I have a one change request (up for discussion)
server/command.go
Outdated
Hint: "(optional)", | ||
Item: "next-week", | ||
}}) | ||
queue.AddTextArgument("Creates a post for user with the given message for the next meeting date.", "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.
queue.AddTextArgument("Creates a post for user with the given message for the next meeting date.", "message", "") | |
queue.AddTextArgument("Message for the next meeting date.", "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.
The text here is describing the meaning of the Message
autocomplete option. I think it makes sense to somehow tell the user Message
is not an autocomplete value, but an input needed from the user.
On the same point, I wonder if we could/should somehow designate the difference between an autocomplete value versus an input required from the user. As an example, next-week
is an autocomplete value and Message
requires free text from the user.
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.
Agree. What we could do is differentiate using icons on the left side, or have different styles, but it is not yet part of the Server PR. I'll create the ticket for this.
I'd suggest using [message]
for now.
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.
That'd be perfect. Thanks!
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.
Is this a backwards compatibly change? Does min_server_version
need to get bumped?
@hanzei There will be no changes for older servers, new functionality will not work of course. No |
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.
Thanks @iomodo! I'm stealing this as a reference for other plugins :)
go.mod
Outdated
@@ -4,8 +4,8 @@ go 1.12 | |||
|
|||
require ( | |||
github.com/blang/semver v3.6.1+incompatible // indirect | |||
github.com/mattermost/mattermost-server v1.4.1-0.20191016162522-6597fdb40134 // Mattermost Server 5.16.0 | |||
github.com/mattermost/mattermost-server/v5 v5.3.2-0.20200525190709-91f010b8b7e8 |
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.
why this version?
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.
This was the latest master, we will switch it to 5.24
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 have tested all the auto-complete functionality
@iomodo one question here... The schedule command functionality has changed instead of |
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.
Tested and passed
- Schedule is now working as expected
- Confirmed setting works with use of auto complete
- Regression tested that you can use partial such as
fri
- Regression tested that week days are case insensitive
LGTM!
Thanks @iomodo fir fixing this.
Summary
This PR will add slash command autocomplete suggestions.