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

[FIX] Add the requested setting for BioLength and NickName #25226

Closed
wants to merge 1 commit into from

Conversation

aakash-gitdev
Copy link
Contributor

@aakash-gitdev aakash-gitdev commented Apr 19, 2022

Added the requested setting for BioLength and NickName also.

Proposed changes (including videos or screenshots)

Issue(s)

Closes #24968

Steps to test or reproduce

Further comments

I want someone to kindly review this PR. Also should I add "i18nLabel" field in the add settings of 'NickName' and 'BioLength'? If yes then any kind of help in finding it's location would be great.

Added the requested setting for BioLength and NickName also.
@dougfabris dougfabris changed the title [FIX] #24968 [FIX] Add the requested setting for BioLength and NickName Apr 19, 2022
Copy link
Member

@gabriellsh gabriellsh left a comment

Choose a reason for hiding this comment

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

There is a couple of things I'd like to point out.

First, answering your question, yes you should add translations when you add a reference to it in a PR.

The other thing is, you added the settings, but you didn't use them. 😛

So, that was mostly a review so far, but I'd like to comment that we shouldn't add setting for this kind of thing. IMO it adds little value and a lot of complexity for maintenance in the future. Instead of adding a setting, you could just add something like const MAX_BIO_LENGTH = 260;. This way, it's no longer a magic number and we don't need a whole setting for it.

Anyway, thanks for the help!

@aakash-gitdev
Copy link
Contributor Author

There is a couple of things I'd like to point out.

First, answering your question, yes you should add translations when you add a reference to it in a PR.

The other thing is, you added the settings, but you didn't use them. 😛

So, that was mostly a review so far, but I'd like to comment that we shouldn't add setting for this kind of thing. IMO it adds little value and a lot of complexity for maintenance in the future. Instead of adding a setting, you could just add something like const MAX_BIO_LENGTH = 260;. This way, it's no longer a magic number and we don't need a whole setting for it.

Anyway, thanks for the help!

Thanks for your review I should have thought of making it a const instead of changing settings. It would be much simpler, I will like to change the code according to your suggestions

@gabriellsh gabriellsh closed this Apr 19, 2022
@gabriellsh
Copy link
Member

closed in favor of #25231

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replacing magic numbers in the code by settings
2 participants