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

[#33661] Missing form field attribute "maxlength". #3510

Closed
wants to merge 1 commit into from
Closed

[#33661] Missing form field attribute "maxlength". #3510

wants to merge 1 commit into from

Conversation

todordev
Copy link
Contributor

This patch adds the attribute "maxlength" to the form field "textarea".

Joomla!Code Tracker Page:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33661

@@ -141,6 +155,7 @@ protected function getInput()
$autocomplete = $autocomplete == ' autocomplete="on"' ? '' : $autocomplete;
$autofocus = $this->autofocus ? ' autofocus' : '';
$spellcheck = $this->spellcheck ? '' : ' spellcheck="false"';
$maxLength = !empty($this->maxLength) ? ' maxlength="' . $this->maxLength . '"' : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not use !empty with maxlength here. If we do we couldn'y set maxLength=0.
use isset in setup method and assign false to maxLength if it is not set. Check rows & columns attributes.

@todordev
Copy link
Contributor Author

I see. I just use the code from the form field "text".
The purpose is to be similar with the code of the "text" field.
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/fields/text.php

I do not want to change the code of the person, which has done the project “Improve Form Fields”. :)
So, I will let you do it if you want. :)

@Achal-Aggarwal
Copy link
Contributor

LOL, I did that project. If you want I can edit both fields and maybe look others also for similar issues. What you say?

@todordev
Copy link
Contributor Author

That will be great! :)
If you want, you can add other attributes.

  • placeholder
  • form
  • selectionDirection
  • selectionEnd
  • selectionStart
  • wrap

Check Valentin's replay on Joomla!Code Tracker Page 33661.

@wilsonge
Copy link
Contributor

placeholder is there. Just it's been called hint

@todordev todordev changed the title Missing form field attribute "maxlength". [#33661] Missing form field attribute "maxlength". Aug 21, 2014
@Robdebert
Copy link

" All is well — The Travis CI build passed " <- is there anything still left open? I refer to this: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33661 where Valentin says, that not enough testing was done and so it does not get implemented in the next version(s)

if you need more code-reviews / testing, i'll be happy to help.

@Bakual
Copy link
Contributor

Bakual commented Nov 10, 2014

Travis is the automated testing. It can detect some code errors, but doesn't detect every error.
We always need manual testing for things to get merged. Usually at least two successful tests should be logged in the issue tracker (http://issues.joomla.org) before something is considered "ready to commit". From there someone will review the code and merge it.

@isidrobaq
Copy link

I tested the proposed patch, and worked as described for me. So here's a "successful test" for this one :).

What's the status for this issue? Maybe would be able to do it into 3.4 if another successful test comes?

Thanks!


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3510.

@infograf768
Copy link
Member

as stated above this needs some small improvements

@zero-24
Copy link
Contributor

zero-24 commented May 3, 2015

@ITPrism can you implement the changes that was suggsested by @Achal-Aggarwal in the inline comments? So we can move this forward? Thanks 😄

@ghost
Copy link

ghost commented May 6, 2015

I think this issue is fixed already by RTC PR #6460 that will be applied in J!3.5.0

@wilsonge
Copy link
Contributor

I have merged #6460. If there are any futher issues can you open a new PR please?

@wilsonge wilsonge closed this May 25, 2015
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.

None yet

9 participants