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

[#17744] - Pressing clear button of calendar on required date field submits admin form in Joomla! 3.7.5 --- corrected #17793

Closed
wants to merge 10 commits into from

Conversation

akritianand
Copy link
Contributor

@akritianand akritianand commented Aug 31, 2017

Pull Request for Issue # 17744

Summary of Changes

Previously, when we edit an article under publishing tab, the clear button in calendar field did not work if it was a required field.
Now, it clears the calendar field when pressed instead of redirecting (as it did originally).
Changes were made in calendar.js and calendar.php

Testing Instructions

Create an admin form with a required date field. i.e. open administrator\components\com_content\models\forms\article.xml and make field "publish_down" required by adding required="true" to field's xml node.
Go to Joomla! administration panel and open an article edit view.
Finish Publishing field should now be a required field.
Add Finish Publishing date, using the date picker of the calendar
Click "Clear" button on the Calendar.

[#17729] - Color of the error message after attempting to delete a Admin-menu - SOLVED
Changed 'alert-success' to 'alert-danger' in line 14
Corrected the functionality of clear button in the finish publishing date in the article setting.
changed calender.min.js to calendar.js as this is where I have made changes.
if (!this.inputField.hasAttribute('required')) {
var savea = row.querySelector('[data-action="clear"]');
savea.addEventListener("click", function (e) {
if (1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we don't need to check for the required attribute? And why if (1)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@akritianand IMHO, the clear button should be hidden when field is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, if that is required, then there was not any problem initially. Because the clear button still showed up if the field was required; it just did not perform its function.

@izharaazmi
Copy link
Contributor

@akritianand You need to generate the calendar.min.js too.

@@ -100,7 +100,7 @@
// The static assets for the calendar
JHtml::_('script', $localesPath, false, true, false, false, true);
JHtml::_('script', $helperPath, false, true, false, false, true);
JHtml::_('script', 'system/fields/calendar.min.js', false, true, false, false, true);
JHtml::_('script', 'system/fields/calendar.js', false, true, false, false, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

please revert this change and update the calendar.min.js file with your changes

@mbabker
Copy link
Contributor

mbabker commented Aug 31, 2017

Also needs updating to deconflict with the now merged #17777

@ghost
Copy link

ghost commented Sep 2, 2017

similar #17809


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

@ghost ghost mentioned this pull request Sep 2, 2017
@izharaazmi
Copy link
Contributor

This pr is broken and we have #17809 for the same issue. This should be closed, imho.

@joomla-cms-bot
Copy link

Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/17793

@ghost
Copy link

ghost commented Sep 3, 2017

closed as stated above.


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

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.

5 participants