? ? ? Pending

User tests: Successful: Unsuccessful:

avatar brianteeman
brianteeman
2 Sep 2020

Weekend is just one word and not two

Therefore it is incorrect to label it in the xml and it should just be <weekend>

Code review

avatar brianteeman brianteeman - open - 2 Sep 2020
avatar brianteeman brianteeman - change - 2 Sep 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 2 Sep 2020
Category Administration Language & Strings Libraries
avatar ChristineWk ChristineWk - test_item - 2 Sep 2020 - Tested successfully
avatar ChristineWk
ChristineWk - comment - 2 Sep 2020

I have tested this item successfully on 2d0b1a1


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

avatar Quy Quy - test_item - 2 Sep 2020 - Tested successfully
avatar Quy
Quy - comment - 2 Sep 2020

I have tested this item successfully on 2d0b1a1


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

avatar Quy Quy - change - 2 Sep 2020
Status Pending Ready to Commit
avatar Quy
Quy - comment - 2 Sep 2020

RTC


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

avatar SharkyKZ
SharkyKZ - comment - 2 Sep 2020

This is a B/C break. Needs to be documented at least.

avatar Quy Quy - change - 2 Sep 2020
Status Ready to Commit Pending
avatar zero-24
zero-24 - comment - 3 Sep 2020

I have just added the docs required label

avatar SharkyKZ
SharkyKZ - comment - 3 Sep 2020

The method can be renamed too.

avatar zero-24
zero-24 - comment - 3 Sep 2020

The method can be renamed too.

hehe that was a draft comment already but seems i have not sended it until now ;)

avatar brianteeman brianteeman - change - 3 Sep 2020
Labels Added: ? ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 3 Sep 2020
Category Administration Language & Strings Libraries Administration Language & Strings Layout Libraries
avatar richard67 richard67 - test_item - 3 Sep 2020 - Tested successfully
avatar richard67
richard67 - comment - 3 Sep 2020

I have tested this item successfully on 1673738

Code review ok. Drone passed, too, after I've restarted it 2 times.


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

avatar ChristineWk ChristineWk - test_item - 3 Sep 2020 - Tested successfully
avatar ChristineWk
ChristineWk - comment - 3 Sep 2020

I have tested this item successfully on 1673738


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

avatar infograf768
infograf768 - comment - 3 Sep 2020

I was sure it would create an error when using the 3.x fr-FR pack for example, but it looks like the parameter is just not used.
Originally, it was supposed to greys the weekend days in the calendar, but, in J4 as well as J3, it keeps using default here which is 0,6 (Saturday and Sunday).
For example when weekends was to be set as Friday, the code would have been <weekend>1</weekend> with this PR, <weekEnd>1</weekEnd> before.

Anyone can confirm?

avatar infograf768
infograf768 - comment - 3 Sep 2020

EDIT: the weekend day greyed in the calendar comes fom the specific js files
i.e. from media/system/js/fields/calendar-locales/whataver.js for each language.

Therefore I have no idea where the xml parameter is used. Any Hint?

avatar infograf768
infograf768 - comment - 3 Sep 2020

I have the feeling it is a remaining of a time when we were not using these js files.

@dgrammatiko
Do you remember?
#11138 (comment)

If this the case, the param may be just deleted in 4.0

avatar brianteeman
brianteeman - comment - 3 Sep 2020

Please create your own issue

avatar infograf768
infograf768 - comment - 3 Sep 2020

Please lets wait for @dgrammatiko to reply. My tests show here that the weekend param can be empty or not existing, which is real weird.

tested modifying en-GB
chnage the en.js to weekend : [1],
modify the metadata to <weekend></weekend> or just delete it

nothing is broken and Friday is set as weekend day in the calendar, as set in the js.

EDIT: If I am right in my tests, it means we do not need to modify the spelling, but can safely delete it.

avatar brianteeman
brianteeman - comment - 3 Sep 2020

It is a different issue - if the weekend settings dont work that is a separate bug that needs to be fixed - please create your own issue!!!

avatar infograf768
infograf768 - comment - 3 Sep 2020

As I said, the param may not need to be used at all. Please cool down and let's first see if the param is necessary.

avatar brianteeman
brianteeman - comment - 3 Sep 2020

Closed - i cant be bothered when you keep bikeshedding issues.

It should be obvious to you that if the weekend code is not working then its a bug as it should work and is an important feature.

avatar brianteeman brianteeman - change - 3 Sep 2020
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-09-03 13:21:07
Closed_By brianteeman
avatar brianteeman brianteeman - close - 3 Sep 2020
avatar infograf768 infograf768 - change - 3 Sep 2020
The description was changed
avatar infograf768 infograf768 - edited - 3 Sep 2020
avatar infograf768
infograf768 - comment - 3 Sep 2020

If you just accepted to test, it should be obvious to you that the parameter which really work for the calendar in core is the one in the .js file and not the one in the metadata. After testing, I am sure we do need the param in the .js. I doubt we still need the metadata one. Is'nt it worth checking that before any change?
The reason is pretty simple: if we merge this PR and it has an impact on some language packs, it would mean we start preparing in 3.10 and backporting there the new method while keeping the older one.
If we just don't need the param anymore, then we just can get rid of it.

avatar dgrammatiko
dgrammatiko - comment - 3 Sep 2020

@brianteeman @infograf768 both of you are right!
This PR needs to be re opened and merged

The problem @infograf768 describes, if it is reproducible then also that needs fixing. IIRC the order for the weekend value was first the XML (language dictated) then if an override existed in the field (XML or layout). If the order is not respected then it's a bug and needs fixing (either in the field/layout or js)

avatar infograf768
infograf768 - comment - 3 Sep 2020

It is only by the .js as of now, including in J3. Never tested via an override in a field or layout.
Could you give examples of these uses?

avatar infograf768
infograf768 - comment - 3 Sep 2020

calendar.js is

				weekend         : JoomlaCalLocale.weekend ? JoomlaCalLocale.weekend : [0,6],
				minYear         : JoomlaCalLocale.minYear ? JoomlaCalLocale.minYear : 1900,
				maxYear         : JoomlaCalLocale.maxYear ? JoomlaCalLocale.maxYear : 2100,
				minYearTmp      : btn.getAttribute("data-min-year"),
				maxYearTmp      : btn.getAttribute("data-max-year"),
				weekendTmp      : btn.getAttribute("data-weekend"),
				time24          : true,
etc.
avatar dgrammatiko
dgrammatiko - comment - 3 Sep 2020

The layout is passing correctly the value:

data-weekend="<?php echo Factory::getLanguage()->getWeekEnd(); ?>"

avatar infograf768
infograf768 - comment - 3 Sep 2020

yes, but not the js

avatar dgrammatiko
dgrammatiko - comment - 3 Sep 2020

The js is missing a couple of lines to check the data attribute, eg

if (btn.getAttribute("data-today-btn")) {
instanceParams.showsTodayBtn = parseInt(btn.getAttribute("data-today-btn")) === 1 ? true : false;
}

avatar infograf768
infograf768 - comment - 3 Sep 2020

What I saw is:
for the weekend from the metadata, we have
weekendTmp : btn.getAttribute("data-weekend"),

then further down

// Evaluate the weekend days
		if (self.params.weekendTmp !== "undefined") {
			self.params.weekend = self.params.weekendTmp.split(',').map(function(item) { return parseInt(item, 10); });
		}

but this self.params.weekend variable is not used.

avatar dgrammatiko
dgrammatiko - comment - 3 Sep 2020

but this self.params.weekend variable is not used.

self.params is becoming this.params later on in the code (this and self refer to the same instance)

So probably

weekend = JoomlaCalLocale.weekend;

should be this.params.weekend

avatar infograf768
infograf768 - comment - 4 Sep 2020

@dgrammatiko
In fact that does not work. The change has to be done here and the metadata value is used.

weekend = JoomlaCalLocale.weekend,

But if the langmetadata <weekend> is empty or absent, we don't fall back to JoomlaCalLocale.weekend

Therefore, shall not we do something like:

    if (this.params.weekend && this.params.weekend > 0) {
      var row = this.tbody.firstChild,
        ar_days = this.ar_days = new Array(),
        weekend = this.params.weekend,
        monthDays = parseInt(date.getLocalWeekDays(this.params.dateType));
    } else {
      var row = this.tbody.firstChild,
        ar_days = this.ar_days = new Array(),
        weekend = JoomlaCalLocale.weekend,
        monthDays = parseInt(date.getLocalWeekDays(this.params.dateType));
    }

In this case JoomlaCalLocale.weekend (the lang.js) is used as fall back and if absent, it defaults to 0,6
(I tested with this.params.weekend.length > 0) but it did not work)

What do you think?

Better code welcome.

avatar dgrammatiko
dgrammatiko - comment - 8 Sep 2020

@infograf768 I wouldn't add conditional so deep in the logic, the evaluation of the weekend value should happen very early. I think this

		if (self.params.weekendTmp !== "undefined") {
			self.params.weekend = self.params.weekendTmp.split(',').map(function(item) { return parseInt(item, 10); });
		}

should be:

		if (self.params.weekendTmp && self.params.weekendTmp !== "") {
			self.params.weekend = self.params.weekendTmp.split(',').map(function(item) { return parseInt(item, 10); });
		}

Add a Comment

Login with GitHub to post a comment