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

Improvement on texts and error behaviours on /todo settings #110

Merged

Conversation

larkox
Copy link
Contributor

@larkox larkox commented Jul 30, 2020

Summary

Improve the texts and error messages/behaviours on /todo settings

Current look:

/todo settings
Screenshot from 2020-07-30 10-04-22
/todo settings summ
Screenshot from 2020-07-30 10-05-01
/todo settings summary
Screenshot from 2020-07-30 10-04-35
/todo settings summary on
Screenshot from 2020-07-30 10-05-41
/todo settings summary off
Screenshot from 2020-07-30 10-05-52
/todo settings summary foo
Screenshot from 2020-07-30 10-06-09
/todo settings summary on foo
Screenshot from 2020-07-30 10-06-50

Ticket Link

None

@larkox larkox added the 1: UX Review Requires review by a UX Designer label Jul 30, 2020
@larkox larkox added this to the v0.4.0 milestone Jul 30, 2020
@larkox larkox requested a review from asaadmahmood July 30, 2020 08:09
@asaadmahmood
Copy link
Contributor

@larkox Can we format the Invalid input on/off like we did above? (in code blocks).

@asaadmahmood
Copy link
Contributor

asaadmahmood commented Jul 30, 2020

Also, we need a full stop after the end of each error.
i.e. "Error: Too many arguments."
And can we make that error text use our error color?
And then we can probably even remove the "Error" at the start.

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2020

Codecov Report

Merging #110 into master will decrease coverage by 0.02%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #110      +/-   ##
=========================================
- Coverage    3.77%   3.75%   -0.03%     
=========================================
  Files           8       8              
  Lines         928     933       +5     
=========================================
  Hits           35      35              
- Misses        893     897       +4     
- Partials        0       1       +1     
Impacted Files Coverage Δ
server/command.go 11.48% <60.00%> (-0.25%) ⬇️

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 bb127b1...2ac1a21. Read the comment docs.

@larkox
Copy link
Contributor Author

larkox commented Jul 30, 2020

@asaadmahmood Sure. Here is a screenshot with the on/off corrected, and a few with the full stop at the end of the sentence.

Screenshot from 2020-07-30 10-30-16

Screenshot from 2020-07-30 10-32-33

@asaadmahmood
Copy link
Contributor

@larkox
And can we make that error text use our error color?
And then we can probably even remove the "Error" at the start.

@larkox
Copy link
Contributor Author

larkox commented Jul 30, 2020

@asaadmahmood We are limited by markdown here. If I remember correctly, there is no markdown to change the color of the text (but would be a nice addition! (as long we keep it to a limited set of colors like error, warning, etc...)).

We could go with Message Attachments, and change the color of the bar on the left, but since we the code is in back-end, I am not sure we can access the theme color for error.

Edit: We can also go with custom post types, but then they will not work on mobile...

Copy link
Contributor

@asaadmahmood asaadmahmood left a comment

Choose a reason for hiding this comment

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

I think we can go with what we have for now then.

@larkox larkox requested review from hanzei and jfrerich July 30, 2020 08:51
@larkox larkox added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester and removed 1: UX Review Requires review by a UX Designer labels Jul 30, 2020
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.

two questions

server/command.go Show resolved Hide resolved
server/command.go Show resolved Hide resolved
@larkox larkox requested a review from hanzei July 30, 2020 11:14
Copy link
Contributor

@jfrerich jfrerich left a comment

Choose a reason for hiding this comment

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

LGTM!

@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Jul 31, 2020
@larkox larkox requested a review from DHaussermann July 31, 2020 08:49
@DHaussermann
Copy link

@hanzei or @jfrerich If you have a bit of time, could one of you peer-test this PR using the peer testing process documented here https://mattermost.atlassian.net/wiki/spaces/INT/pages/619937892/Peer+Testing+Process

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
Based on code changes, I have reviewed improved error messages shown to the user. No issues found
LGTM!

No changes needed to release testing.

@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 Aug 31, 2020
@larkox larkox merged commit ccfcd25 into mattermost-community:master Sep 2, 2020
@larkox larkox mentioned this pull request Sep 7, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants