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

fix(#150): add maxlength on input #222

Merged
merged 2 commits into from
Apr 23, 2024
Merged

fix(#150): add maxlength on input #222

merged 2 commits into from
Apr 23, 2024

Conversation

jufab
Copy link
Contributor

@jufab jufab commented Oct 20, 2023

Summary

Add a Max Length on user input.
Max length = 22

Ticket Link

Fixe #150

@jufab jufab requested a review from larkox as a code owner October 20, 2023 00:31
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3664f0f) 6.42% compared to head (5c3d54e) 6.42%.

Additional details and impacted files 
@@          Coverage Diff           @@
##           master    #222   +/-   ##
======================================
  Coverage    6.42%   6.42%           
======================================
  Files          11      11           
  Lines        1712    1712           
======================================
  Hits          110     110           
  Misses       1594    1594           
  Partials        8       8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mickmister
mickmister reviewed Oct 20, 2023
View reviewed changes
Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @jufab 🎉

Mostly looks good, I have just one request to use type inference to avoid the use of @ts-ignore plus a few small comments. Otherwise LGTM 👍

webapp/src/components/user_selector/autocomplete_selector.tsx Outdated
@@ -16,6 +16,7 @@ import {FormatOptionLabelContext} from 'react-select/src/Select';
import {getColorStyles, getDescription, getProfilePicture} from '../../utils';

import './autocomplete_selector.scss';
import {components} from 'react-select';
Copy link
Contributor

Choose a reason for hiding this comment

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

This import should be further up with the other react-select imports. This should really be caught by the linter but I think that rule is not set up for this project

webapp/src/components/user_selector/autocomplete_selector.tsx Outdated
@@ -30,6 +31,7 @@ type Props = {
theme: Theme,
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Unnecessary extra line here

webapp/src/components/user_selector/autocomplete_selector.tsx Outdated
Comment on lines 121 to 122
//@ts-ignore
const Input = (props) => <components.Input {...props} maxLength={22} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we don't need to use @ts-ignore. I think we can inline the declaration of this below when defining the components prop, and typescript will automatically infer the type of props

components={{
    Input: (props) => <components.Input {...props} maxLength={22} />,
}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I inline this code, I have an other problem. 😢

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay then it seems the types for the library don't agree with its supported functionality. Yeah let's just use @ts-ignore here. Thanks for trying this 👍

@mickmister
Copy link
Contributor

Also there are a few linting issues being reported by CI:
https://github.com/mattermost/mattermost-plugin-todo/actions/runs/6581802885/job/17902038384?pr=222#step:4:8620

Error:    19:1   error  `react-select` import should occur before import of `../../utils`  import/order
Error:    34:1   error  More than 1 blank line not allowed                                 no-multiple-empty-lines
Error:   122:20  error  'props' is already declared in the upper scope                     no-shadow
Error:   122:59  error  Prop `maxLength` must be placed on a new line                      react/jsx-max-props-per-line
Error:   122:74  error  A space is forbidden before closing bracket                        react/jsx-tag-spacing

Can you please take a look at these errors @jufab? Thank you!

@mickmister mickmister added the 3: QA Review Requires review by a QA tester label Oct 23, 2023
@avas27JTG avas27JTG added this to the v1.0.0 milestone Dec 22, 2023
AayushChaudhary0001
AayushChaudhary0001 approved these changes Apr 15, 2024
View reviewed changes
Copy link

@AayushChaudhary0001 AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

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

Tested this PR for the following scenarios:-

  • Username field can accept maximum length upto 22 characters while creating a ToDo.
  • Username field can accept maximum length upto 22 characters while assigning a ToDo.

The PR was found working fine for both the above conditions, LGTM. Approved.

@mickmister mickmister added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Apr 23, 2024
@mickmister
Copy link
Contributor

Thanks @jufab!!

@mickmister mickmister merged commit b8dff94 into mattermost-community:master Apr 23, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AayushChaudhary0001 AayushChaudhary0001 AayushChaudhary0001 approved these changes

@mickmister mickmister mickmister approved these changes

@larkox larkox larkox approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@jufab @codecov-commenter @mickmister @larkox @AayushChaudhary0001 @avas27JTG