-
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
feat: default hashtag includes channel name #30
feat: default hashtag includes channel name #30
Conversation
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.
LGTM. Would you mind adding an update to the README letting users know what the default will be?
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 approach taken in this PR leads to incorrect results, explaination and examples in #10 (comment).
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 for the contribution @will7200 !
Small change request bellow
BREAKING CHANGE: Meeting.HashtagFormat's DateTime formatting must be wrapped in double braces for the plugin to generate the correct datetime 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.
Thanks @will7200!
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 @will7200! Apologies on the delay on this review. Everything looks good, great tests!
Just wanted to include a request to add the new requirement for date format
to the help text in the settings modal here
Maybe adding:
Embed a date writing down what January 2, 2006 would look like within double curly braces, i.e. {{Jan02}}
Codecov Report
@@ Coverage Diff @@
## master #30 +/- ##
===========================================
+ Coverage 16.46% 28.27% +11.81%
===========================================
Files 6 6
Lines 328 343 +15
===========================================
+ Hits 54 97 +43
+ Misses 262 229 -33
- Partials 12 17 +5
Continue to review full report at Codecov.
|
@marianunez I was finally able to get around this, thank you for your patience. I added as instructed. |
@will7200 Heads up that I merged |
@hanzei Fixed |
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 @will7200. LGTM after @jfrerich
suggestions and resolving the merge conflicts below.
Co-authored-by: Jason Frerich <jason.frerich@mattermost.com>
@will7200 Could you please resolve the merge conflicts? |
@hanzei done |
Thanks @will7200 👍 |
@jfrerich Could you give this PR a final review? |
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.
LGTM! Thanks again, @will7200!
@marianunez @DHaussermann Should this PR get QA reviewed or can we merge it as it is? |
If we do skip the QA review here, I would suggest a quick smoke test to the release before deploying to community. |
Summary
Closes #10
Default hashtag to use the first 15 characters of a channel name prefixed to the date format.
Ticket Link
JIRA: https://mattermost.atlassian.net/browse/MM-22727