Skip to content

[4.0] Meta Description character count#27804

Merged
wilsonge merged 11 commits intojoomla:4.0-devfrom
brianteeman:counter2
Feb 6, 2020
Merged

[4.0] Meta Description character count#27804
wilsonge merged 11 commits intojoomla:4.0-devfrom
brianteeman:counter2

Conversation

@brianteeman
Copy link
Copy Markdown
Contributor

@brianteeman brianteeman commented Feb 4, 2020

redo of #25262 and #27359

A request was made to have a character count when entering a meta description. I have done this and applied it to the Site Meta Description in the global configuration as shown in the screenshot and all the meta description fields)

image

This can be applied to any textarea field by adding charcounter="true" to the field definition in the xml.

The character count has a default maximum of 200 characters but this can be configured in the field xml by adding maxlength="160"

Accessibility is obviously very important and this PR addresses this as well. The meta description text is announced when the user focuses on the input together with the string to indicate the characters used. Obviously a live count announced by the screenreader would be distracting so it waits 2000 milliseconds before updating the user with the screen reader user with the current count.

The string 86 characters remaining of 160 characters. is a regular joomla language string except the numbers are replaced by the script. In this example I chose
"{remaining} characters remaining of {maxlength} characters.". Available placeholders are {remaining}, {maxlength}, {length}

Documentation

Needs updating to document the new charcounter option

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Feb 4, 2020
Comment thread layouts/joomla/form/field/textarea.php Outdated
Comment thread layouts/joomla/form/field/textarea.php Outdated
@richard67
Copy link
Copy Markdown
Member

@brianteeman I just tested with your last change for the asset structure. I get following JS error when opening global config:

TypeError: menuToggleIcon is null, file admin-menu.js, line 72, column 7.

@brianteeman
Copy link
Copy Markdown
Contributor Author

yeah - something isn't right - but too tired now - will look again in the morning

@richard67
Copy link
Copy Markdown
Member

I can continue testing tomorrow evening after work.

@richard67
Copy link
Copy Markdown
Member

Btw. my suggested changes are still valid. Last comment that you can forget about them applies only if value of variable “$class” has already a space at the end, but I don’t think that’s the case. Comment clearly says that but for a quick reader it might be less clear. Maybe my changes will even fix the js problem because they will fix css classes of the html element?

@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Feb 5, 2020

@richard67 I am stepping through the code to see what causes the break - currently I am getting an error without the js changes I made
Undefined method enableAsset in class Joomla\CMS\WebAsset\WebAssetManager

~So I am guessing that something went wrong with the conflict fix. ~ guessed wrong

@richard67
Copy link
Copy Markdown
Member

@brianteeman Without this PR I don't get any JS error, and with this PR I get it only when going to Global Configuration where the character count is used.

What if the short-and-sweet js does a DOM query for css class "charcount" and doesn't find it due to the missing space between class names which I mentioned in my suggested changes, and there is no error handling?

I strongly suggest to apply my changes for a test and then see what happens.

@brianteeman
Copy link
Copy Markdown
Contributor Author

I have fixed the space - it was a valid issue but not related to the problem you saw

I have switched to using htmlhelper to load the script for now as I couldnt work out how to use the webasset stuff.

As this is for review as a concept I think thats ok for now

thanks for all your help

Comment thread layouts/joomla/form/field/textarea.php Outdated
Comment thread layouts/joomla/form/field/textarea.php
Comment thread layouts/joomla/form/field/textarea.php Outdated
brianteeman and others added 2 commits February 5, 2020 16:30
Co-Authored-By: Quy <quy@fluxbb.org>
Co-Authored-By: Quy <quy@fluxbb.org>
@richard67
Copy link
Copy Markdown
Member

@brianteeman No, thank YOU for all the work you do for Joomla .. I would be an a..hole if I could help but would not do.

Is it ready for testing?

@brianteeman
Copy link
Copy Markdown
Contributor Author

Yes

@richard67
Copy link
Copy Markdown
Member

Should we ping Fedik to check if there is an issue with the web asset manager?

@richard67
Copy link
Copy Markdown
Member

Ah, and by the way: Could we display the counter in hex instead of decimal? Those template ranters say our backend is made by developers for developers, and I don't wanna disappoint them ;-) (joking)

@brianteeman
Copy link
Copy Markdown
Contributor Author

Can't do hex but I can do binary

@richard67
Copy link
Copy Markdown
Member

Even better ;-)

@richard67
Copy link
Copy Markdown
Member

I have tested this item ✅ successfully on 483db5b

Works like a charm and looks good.


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

Comment thread build/build-modules-js/settings.json Outdated
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 5, 2020
@brianteeman
Copy link
Copy Markdown
Contributor Author

@Quy language issues should be resolved now

* use WebAsset

* fix js builder, to not lose attributes
@brianteeman
Copy link
Copy Markdown
Contributor Author

Please retest as @Fedik has provided the correct js changes to use webassets

@richard67
Copy link
Copy Markdown
Member

Will retest tonight after work (CET). Thanks @Fedik .

@jwaisner
Copy link
Copy Markdown
Member

jwaisner commented Feb 6, 2020

After applying this PR the article layout is messed up and I am showing errors.

27804

I ran npm ci as well. I also applied this PR via Patchtester and the diff.

Not sure if this a problem, but I noticed that the package.json and package-lock.json files were modified when applying the PR.

@richard67
Copy link
Copy Markdown
Member

Not sure if this a problem, but I noticed that the package.json and package-lock.json files were modified when applying the PR.

@jwaisner Is not a problem, is desired. This PR modifies them because introducing a new 3rd party npm package named "short-and-sweet" for the character counting.

Co-Authored-By: Quy <quy@fluxbb.org>
@Quy
Copy link
Copy Markdown
Contributor

Quy commented Feb 6, 2020

I have tested this item ✅ successfully on a73e030


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

@jwaisner
Copy link
Copy Markdown
Member

jwaisner commented Feb 6, 2020

I still have the same issue after the latest commit... I am backing out of testing this one as it is apparent I am unable to understand why this PR behaves any differently than others I have tested.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Feb 6, 2020

@jwaisner The message said that there is no asset (js or css) for the short-and-sweet package. Which version of patchtester did you use? Latest 4 beta still has issues when having a PR like this here which changes package-lock.json. Or 3? If 3: Have you run npm install? It will see the changed package.json and fetch the package and later build and copy the assets includinh the ones of this new package, and then it will update package-lock.json, too, if necessary (but should not be neccessary when using the package-lock.json from this PR because there it is all included already).

@jwaisner
Copy link
Copy Markdown
Member

jwaisner commented Feb 6, 2020

@richard67 I am using 4.0 beta 2. Should I fall back to 3.0?


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

@richard67
Copy link
Copy Markdown
Member

richard67 commented Feb 6, 2020

@jwaisner If you have npm: Yes, then apply patch and run "npm install". Then clear cache just to be sure. or switch off CI integration, then it should behave like pt 3.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Feb 6, 2020

I have tested this item ✅ successfully on a73e030

  • Global Config - Site
  • Category
  • Contact
  • Article
  • Content Language
  • Newsfeed
  • Tags

All ok.

No errors in PHP log or browser console, no unwanted side effects noticed.


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

@richard67
Copy link
Copy Markdown
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 6, 2020
@wilsonge wilsonge merged commit 6b56d8a into joomla:4.0-dev Feb 6, 2020
@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Feb 6, 2020

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 6, 2020
@wilsonge wilsonge added this to the Joomla 4.0 milestone Feb 6, 2020
@brianteeman
Copy link
Copy Markdown
Contributor Author

Woohoo. Thanks all!!!

@brianteeman brianteeman deleted the counter2 branch February 6, 2020 22:02
@richard67
Copy link
Copy Markdown
Member

@brianteeman Shall I now make the PR to change to binary numbers?

@kernusr
Copy link
Copy Markdown
Contributor

kernusr commented Mar 5, 2020

@brianteeman Do you put a strict limit on the maximum length of the field? This will not save the form if the meta-description is longer than the set field length?
If so, then this approach is not true!
The limit on the maximum number of characters in the meta description, as in the browser header, is a recommendation and should not limit the user

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants