User tests: Successful: Unsuccessful:
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
Status | New | ⇒ | Pending |
Category | ⇒ | Administration Language & Strings Libraries |
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
This is a B/C break. Needs to be documented at least.
Status | Ready to Commit | ⇒ | Pending |
I have just added the docs required label
The method can be renamed too.
The method can be renamed too.
hehe that was a draft comment already but seems i have not sended it until now ;)
Labels |
Added:
?
?
?
|
Category | Administration Language & Strings Libraries | ⇒ | Administration Language & Strings Layout Libraries |
I have tested this item
Code review ok. Drone passed, too, after I've restarted it 2 times.
I have tested this item
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?
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?
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
Please create your own issue
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.
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!!!
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.
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.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-09-03 13:21:07 |
Closed_By | ⇒ | brianteeman |
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.
@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)
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?
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.
The layout is passing correctly the value:
yes, but not the js
The js is missing a couple of lines to check the data attribute, eg
joomla-cms/build/media_source/system/js/fields/calendar.es5.js
Lines 101 to 103 in fb12628
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.
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
this.params.weekend
@dgrammatiko
In fact that does not work. The change has to be done here and the metadata value is used.
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.
@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); });
}
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.