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.

Sign up for GitHub

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

Jump to bottom

Add autocomplete #69

Merged
merged 5 commits into from
Jun 18, 2020
Merged

Add autocomplete #69

merged 5 commits into from
Jun 18, 2020

Conversation

iomodo
Copy link
Contributor

@iomodo iomodo commented May 27, 2020

Summary

This PR will add slash command autocomplete functionality to a Todo plugin.

@iomodo iomodo requested a review from larkox as a code owner May 27, 2020 13:59
@codecov-commenter
Copy link

codecov-commenter commented May 27, 2020 •
edited
Loading

Codecov Report

Merging #69 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #69   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           8       8           
  Lines         829     856   +27     
======================================
- Misses        829     856   +27
Impacted Files Coverage Δ
server/command.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d90daeb...95458bb. Read the comment docs.

@iomodo iomodo requested a review from hanzei May 27, 2020 14:00
@iomodo iomodo added the 2: Dev Review Requires review by a core committer label May 27, 2020
@hanzei hanzei added the 3: QA Review Requires review by a QA tester label May 27, 2020
This was referenced May 28, 2020
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Open
Closed
Closed
Closed
hanzei
hanzei suggested changes May 28, 2020
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Awesome to see these improvements 🎉

server/command.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/command.go Show resolved Hide resolved
server/command.go
todo.AddCommand(pop)

send := model.NewAutocompleteData("send", "[user] [todo]", "Sends a Todo to a specified user")
send.AddTextArgument("Whom to send", "[@awesomePerson]", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

While testing this I've noticed that after picking a user from the @ mention picker the autocomplete stays closed. Should I file a JIRA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a known issue and is run by UX as well as @aaronrothschild. If you press space autocomplete will continue suggesting. Fix will need a rework of the at_mention_provider as well as suggestions component.

Copy link
Contributor

Choose a reason for hiding this comment

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

@iomodo Is there a JIRA that we can link here for reference?

larkox
larkox approved these changes Jun 1, 2020
View reviewed changes
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

LGTM, just a small question about the version.

go.mod Outdated
@@ -3,7 +3,7 @@ module github.com/mattermost/mattermost-plugin-todo
go 1.13

require (
github.com/mattermost/mattermost-server/v5 v5.20.0
github.com/mattermost/mattermost-server/v5 v5.3.2-0.20200527124843-dea705969ca4
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this is targetting certain commit or branch. Couldn't we target a proper version? (5.23? 5.24?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we will target 5.24 when it is cut.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/mattermost/mattermost-server/releases/tag/v5.24.0-rc1 can be used

@jfrerich jfrerich mentioned this pull request Jun 3, 2020
Closed
@larkox larkox requested a review from hanzei June 4, 2020 13:12
hanzei
hanzei approved these changes Jun 8, 2020
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

I'm not entirely convinced on the UX of /todo list, but that's fine to fix in another PR.

@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Jun 8, 2020
@hanzei hanzei mentioned this pull request Jun 8, 2020
Merged
@hanzei hanzei requested a review from DHaussermann June 8, 2020 11:15
This was referenced Jun 9, 2020
Closed
Open
@NateWilliams2 NateWilliams2 mentioned this pull request Jun 11, 2020
Merged
DHaussermann
DHaussermann approved these changes Jun 18, 2020
View reviewed changes
Copy link

@DHaussermann DHaussermann left a comment •
edited by hanzei
Loading

Choose a reason for hiding this comment

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

Tested and passed

  • All commands work
  • All commands are correct and description is Accurate
  • Regression tested plugin on previous server version without auto-complete for proper functionality

LGTM!

@DHaussermann
Copy link

Thanks @iomodo for updating this one.

Since ToDo has a command that takes an optional argument at the end of the command, I did notice a concern with having to type space twice to remove the list. Maybe it's just me but, I find that this may not be obvious to users who want to send without either argument. As this is not specific to any plugin I wrote a Jira task here https://mattermost.atlassian.net/browse/MM-26282 please share any feedback you may have.

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jun 18, 2020
@hanzei hanzei added this to the v0.3.0 milestone Jun 18, 2020
@hanzei hanzei merged commit 2c70ff0 into master Jun 18, 2020
@hanzei hanzei deleted the add-autocomplete branch June 18, 2020 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanzei hanzei hanzei approved these changes

@larkox larkox larkox approved these changes

@DHaussermann DHaussermann DHaussermann approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@iomodo @codecov-commenter @DHaussermann @larkox @hanzei