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.

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

[GH-118] Add slash command autocomplete functionality #143

Conversation

ByeongsuPark
Copy link
Contributor

Summary

This PR adds slash command autocomplete functionality.

Ticket Link

Fixes #118

@ByeongsuPark ByeongsuPark requested a review from levb as a code owner October 25, 2020 12:01
@mattermod
Copy link
Contributor

Hello @ByeongsuPark,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@levb
Copy link
Contributor

levb commented Oct 25, 2020

@ByeongsuPark, thank you for the contribution! Do you think you can also add dynamic select support, as appropriate, to select the links to view/edit? It has been one of the biggest usability issues with the command, especially for links that do not have names. If you prefer, it can be done as a separate PR?

@levb levb requested a review from iomodo October 25, 2020 12:08
@levb levb added the 2: Dev Review Requires review by a core committer label Oct 25, 2020
@ByeongsuPark
Copy link
Contributor Author

ByeongsuPark commented Oct 26, 2020

Hi, @levb. I'm not sure whether I can implement the feature you explained for now. How about open a new issue about the dynamic select support feature so that someone else also can work on the issue?

@hanzei hanzei added the 3: QA Review Requires review by a QA tester label Oct 26, 2020
@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

@jfrerich
Copy link
Contributor

jfrerich commented Nov 2, 2020

@levb, would you mind creating the new GitHub issue and reviewing this PR as is?

Copy link
Contributor

@iomodo iomodo left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@iomodo iomodo removed the 2: Dev Review Requires review by a core committer label Nov 20, 2020
@hanzei hanzei requested a review from DHaussermann December 2, 2020 10:37
@hanzei hanzei added this to the v1.3.0 milestone Dec 2, 2020
@hanzei hanzei mentioned this pull request Dec 2, 2020
5 tasks
@DHaussermann
Copy link

DHaussermann commented Dec 10, 2020

@ByeongsuPark Thanks for this enhancement. This makes for a huge usability improvement!

I have now done a round of testing and this is mostly working as expected. I see that the test command was left out which to me, seems like it may have been intentional. But, there are a couple places where the auto-complete functionality is incomplete.

  1. When using set, Scope should be one of the available options. ex /autolink set jira Scope MyTeam
  2. When using list it is also possible to get results based on the value of Template and Pattern so, in addtion to [name] these should appear as optional for commands such as /autolink list Pattern mmtest or /autolink list Template ${jira_id}

This PR is a great addition as is, but let me know if your able to add these 2 points as well.

@ByeongsuPark
Copy link
Contributor Author

@DHaussermann Yes, I can add it. I didn't commit it yet but Should the result look like the images below?

스크린샷, 2020-12-15 16-57-41
스크린샷, 2020-12-15 16-54-26

@DHaussermann
Copy link

@ByeongsuPark Sorry for the late response. These look good.
One small thing is missing for consistency with this and other plugins. In the 1st screen - The options for Template and Scope should also include the (optional) beside them the same way [name] already has.

Thanks for taking the time to further improve this. Much appreciated.

'Scope' is now one of the autocomplete list items in 'set' command.
Also, 'Template' and 'Pattern' option added to the autocomplete list item of 'list' command.
@codecov-io
Copy link

codecov-io commented Dec 28, 2020

Codecov Report

Merging #143 (53cd8f7) into master (fa403a0) will decrease coverage by 5.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   45.04%   40.03%   -5.02%     
==========================================
  Files           6        6              
  Lines         535      602      +67     
==========================================
  Hits          241      241              
- Misses        278      345      +67     
  Partials       16       16              
Impacted Files Coverage Δ
server/autolinkplugin/config.go 33.59% <0.00%> (-36.90%) ⬇️

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 fa403a0...53cd8f7. Read the comment docs.

@ByeongsuPark
Copy link
Contributor Author

@DHaussermann I fixed and commit it. Now it looks like the image below.
Also, thanks to your comment, I registered the missed test autocomplete command data
스크린샷, 2020-12-28 17-26-59

@DHaussermann
Copy link

Thanks @ByeongsuPark but, I do see one last minor issue in the screenshot. Scope and Pattern do not take the [] . This should be unchanged from before as these are options you must select. name does have the [] because this is the value that will be used if you start to type in some text without selecting anything.

Sorry if my last comment was not completely clear. The UX for Autolink is a bit different from other plugins so I'm curious if it's trivial for you to make this change?

@ByeongsuPark
Copy link
Contributor Author

ByeongsuPark commented Jan 5, 2021

@DHaussermann I can make it change. But I have a question.
For autocomplete command for list,
Is Template what you want to mention instead of Scope?
I'm confused because you said that Scope does not take the [].
Please let me know if you spot any misunderstanding.

@DHaussermann
Copy link

@ByeongsuPark yes, exactly. Sorry I made an error when adding the comment :)

Template and Pattern are what must be changed. I have now edited my comment to correct it.

Side note for clarity: Scope is not available when using list and on your last commit Scope looks correct when using set so no changes are needed for Scope

@ByeongsuPark
Copy link
Contributor Author

ByeongsuPark commented Jan 15, 2021

@DHaussermann
I've committed a fix.
Pattern and Scope now does not take [].

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Reviewed all commands including changes to list subcommand
  • Autocomplete now matches convention in all cases
  • Tested in Mobile client as well as a precaution
  • No issues found

LGTM!

@ByeongsuPark thank you so much for your continued efforts on this to implement the remaining changes. Your contribution is very much appreciated!

@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 Jan 18, 2021
@hanzei hanzei merged commit c9ee2eb into mattermost-community:master Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Hacktoberfest hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add slash command autocomplete functionality
8 participants