Skip to content

Conversation

@bembelimen
Copy link
Contributor

Summary of Changes

Enabled character counter for text fields

Testing Instructions

  1. Apply patch
  2. NPM i
  3. Open components/com_users/forms/registration.xml
  4. Add the following attributes to the "name" field:
    maxlength="30" charcounter="true"
  5. Go to the frontend registration and check the name field

Actual result BEFORE applying this Pull Request

Nothing different

Expected result AFTER applying this Pull Request

grafik

grafik

@brianteeman
Copy link
Contributor

Could this option be added to the custom fields as well?

@bembelimen
Copy link
Contributor Author

bembelimen commented Sep 27, 2021

Could this option be added to the custom fields as well?

In theory, it could be added to any (input/textarea) field. Currently I activate it manually in my override via:
$form->setFieldAttribute('fieldname', 'charcounter', 'true', 'com_fields'); and it works "out-of-the-box".

But that's currently a "bigger" problem in Joomla! (in my opinion): we use for every field an unique layout (which is good on one side) but therefore are very inconsistent over all (like the suffix/prefix info are only available for normal text fields and not for e.g. number fields etc.).
Probably worth to have there a nicer solution.

@brianteeman
Copy link
Contributor

like the suffix/prefix info are only available for normal text fields and not for e.g. number fields etc.

That is a bug that should be fixed ;)

@bembelimen
Copy link
Contributor Author

like the suffix/prefix info are only available for normal text fields and not for e.g. number fields etc.

That is a bug that should be fixed ;)

Yes and no, it's failure by design :) the number field does not extend the text field and the layouts could be one base layout with just different attributes....But I guess this counts for many fields (like all dropdowns etc.)

@brianteeman
Copy link
Contributor

it's failure by design :)

or by omission

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 97294eb


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

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Sep 27, 2021

@bembelimen I wouldn't enable that before fixing the counter to work nicely for repeatable fields. BTW the project should review the fields compatibility with repeatable fields and patch what's broken (ie a field with a counter in a repeatable will work only for the first instance, all others will miss the counter because there's no event to trigger the initialization...)

@bembelimen
Copy link
Contributor Author

Hi @dgrammatiko do you mean this PR (as the problem is the same with textareas then) or the spreading over all fields?

@dgrammatiko
Copy link
Contributor

as the problem is the same with textareas

Yes, the initialization of that script is missing the listener for subform add/removal part

the spreading over all fields?

There are many core fields that have issues or not working at all in subforms. Some are easy to patch (ie this one) others need some more changes

@brianteeman
Copy link
Contributor

it will be more of an issue with this field - and I see an issue whereby the capabilities of a custom field are not in sync with an xml field.

I agree that all fields need to be reviewed with subforms - there are already a few open issues but thats beyond the scope here (I think)

@dgrammatiko
Copy link
Contributor

it will be more of an issue with this field - and I see an issue whereby the capabilities of a custom field are not in sync with an xml field.

Probably the repeatable versions of input and textarea need also an update to expose this feature (if it's not already exposed, I haven't checked the code, I was commenting on the base that the core fields should play nicely with repeatables).
Anyways I provided the code needed

@brianteeman
Copy link
Contributor

brianteeman commented Sep 29, 2021

But that's currently a "bigger" problem in Joomla! (in my opinion): we use for every field an unique layout (which is good on one side) but therefore are very inconsistent over all (like the suffix/prefix info are only available for normal text fields and not for e.g. number fields etc.).

I just checked. Suffix and Prefix are available for every field - it is in the render options

image

image

@RickR2H
Copy link
Member

RickR2H commented Sep 30, 2021

I have tested this item ✅ successfully on 97294eb


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 3, 2021
@dgrammatiko
Copy link
Contributor

@richard67 my comment is still valid, introducing this without support for repeatable fields is plain wrong 😑

@richard67
Copy link
Member

@richard67 my comment is still valid, introducing this without support for repeatable fields is plain wrong 😑

@dgrammatiko Sorry, was not clear to me that there is something remaining to be done since the PR has got 2 test.

@bembelimen What's the status of this PR? You want to leave it as it is, or do you want to adapt it to support repeatable fields?

@richard67
Copy link
Member

Back to pending due to ongoing clarifications.


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

@richard67 richard67 removed the RTC This Pull Request is Ready To Commit label Oct 3, 2021
@richard67
Copy link
Member

Needs review.


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

@chmst chmst changed the base branch from 4.0-dev to 4.1-dev January 31, 2022 16:37
@mgscreativa
Copy link

Hi! just using the brand new charcounter param in my edit message form like this

	<field
		name="title"
		type="text"
		label="COM_JAPPPUSH_FIELD_MESSAGE_TITLE_LABEL"
		maxlength="35"
		charcounter="true"
		required="true"
	/>

	<field
		name="body"
		type="textarea"
		label="COM_JAPPPUSH_FIELD_MESSAGE_BODY_LABEL"
		maxlength="160"
		charcounter="true"
	/>

Is it possible to apply this to the text input?

Thanks!


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

@richard67
Copy link
Member

Is it possible to apply this to the text input?

@mgscreativa Isn't that exactly what this pull request (PR) here here is doing?

If so, please test this PR and then mark your test result by going to the issue tracker here https://issues.joomla.org/tracker/joomla-cms/35678 and then using the blue "Test this" button at the top left corner, selecting the appropriate test result and submitting.

Every PR needs 2 successful tests by a human to get accepted, so if this PR solves your problem then please test it as described to bring it forward. Thanks in advance.

@mgscreativa
Copy link

I have tested this item ✅ successfully on ae922dc

Tested ok, no issues!


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

@mgscreativa
Copy link

Hi @richard67 done testing, works ok! Hope it gets in J 4.1


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

Copy link

@mgscreativa mgscreativa 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 it works just fine!

@RickR2H
Copy link
Member

RickR2H commented Jul 29, 2022

@bembelimen After building I get the error: Uncaught SyntaxError: missing ) after argument list (at short-and-sweet.min.js?1.0.4:10:104)
Don't know if it is related, but the counter does not show up. Could you take a look?

@richard67
Copy link
Member

@bembelimen After building I get the error: Uncaught SyntaxError: missing ) after argument list (at short-and-sweet.min.js?1.0.4:10:104)
Don't know if it is related, but the counter does not show up. Could you take a look?

Previous review comment already mentioned that. There is indeed a syntax error in the js. The closing single quote of a query selector string is missing.

@mgscreativa
Copy link

Hi! is there anything else I can do to helpo this PR to pass? I'm using it in my site and it works just fine.

Thanks!

@richard67
Copy link
Member

richard67 commented Aug 15, 2022

As long as @bembelimen does not fix the syntax error in the JS mentioned above, it can not be accepted.

@mgscreativa
Copy link

Hi @richard67 can I send another PR with the same changes to try to pass it through?

@mgscreativa
Copy link

Hi! @bembelimen would yo be so kind to review this typo here to be able to pass this PR? https://github.com/joomla/joomla-cms/pull/35678/files#r926994733

Thanks!

@bembelimen bembelimen changed the base branch from 4.2-dev to 4.3-dev August 17, 2022 23:12
@mgscreativa
Copy link

mgscreativa commented Aug 26, 2022

Hi @richard67 Can I send a fresh new PR to pass this new feature? Because the original PR author doesn't respond at all...

@richard67
Copy link
Member

Hi @richard67 Can I send a fresh new PR to pass this new feature? Because the original PR author doesn't responds at all...

@mgscreativa Let's wait a bit more. I've just notified him again.

@obuisard
Copy link
Contributor

I have tested this item ✅ successfully on e13df24


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

1 similar comment
@sdwjoomla
Copy link
Contributor

I have tested this item ✅ successfully on e13df24


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

@obuisard obuisard added this to the Joomla! 4.3.0 milestone Sep 24, 2022
@obuisard obuisard merged commit f19fa75 into joomla:4.3-dev Sep 24, 2022
@obuisard
Copy link
Contributor

Thank you Benjamin @bembelimen!

@obuisard
Copy link
Contributor

Documentation found under https://docs.joomla.org/Text_form_field_type

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.