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

[4.0] RFC Quickicons Joomla and Extensions updates #23095

Merged
merged 9 commits into from Nov 28, 2018

Conversation

infograf768
Copy link
Member

Replaces and complete #23061 (for what concerns Joomla Update).

Implement NOTE in #23061 (comment)

Summary of Changes

  1. Adding Joomla Version
  2. Change icon color to warning when an update is available as danger is used in case of error.
  3. Place the Update Now and View Updates buttons to the bottom right of the Message, centering the rest of the message.
  4. Adapt Atum CSS concerned to RTL, including the joomla-alert
  5. Correct unnecessary , and : in the language strings
  6. Correct Text::script to false by default to let single quotes in language strings to display unescaped instead of \'

Testing Instructions

To test on 4.0-dev branch

Concerning joomlaupdate:
Modify version.php line 63 to const EXTRA_VERSION = 'alpha4-dev';
Go to database and Fix database.
Then in JoomlaUpdate Options choose Custom URL and add this link (Thanks Tobias) :
https://update.joomla.org/core/nightlies/next_major_list.xml
Save Options.
Display Control Panel.

Concerning extensions update:
Install an older language pack in order to get a warning for an update.
fr-FR_joomla_lang_full_3.9.0v1.zip

in both cases, test also RTL by installing and switching to Persian language.

Test before patch, then run npm ci

Expected result

screen shot 2018-11-16 at 11 22 24

screen shot 2018-11-16 at 09 31 19

screen shot 2018-11-16 at 09 30 00

screen shot 2018-11-16 at 11 45 28

@tuum @wilsonge


// Quickicon specific
.message-alert {
text-align: left !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

surely there is a way to do this without using !important

Copy link
Member Author

Choose a reason for hiding this comment

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

This is what is used in node_modules and vendor:

@each $breakpoint in map-keys($grid-breakpoints) {
  @include media-breakpoint-up($breakpoint) {
    $infix: breakpoint-infix($breakpoint, $grid-breakpoints);

    .text#{$infix}-left   { text-align: left !important; }
    .text#{$infix}-right  { text-align: right !important; }
    .text#{$infix}-center { text-align: center !important; }
  }
}

and ported to /administrator/templates/atum/css/bootstrap.css once npm ci has run:

.text-left {
  text-align: left !important; }

.text-right {
  text-align: right !important; }

.text-center {
  text-align: center !important; }

Copy link
Contributor

@brianteeman brianteeman left a comment

Choose a reason for hiding this comment

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

punctuation as commented inline

Copy link
Contributor

@brianteeman brianteeman left a comment

Choose a reason for hiding this comment

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

approve language strings

@richard67
Copy link
Member

I have tested this item ✅ successfully on a419403


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

@infograf768
Copy link
Member Author

Note: furher PR will correct RTL for the J version

@infograf768
Copy link
Member Author

@richard67
I need to update this now for RTL version but also for another js issue.
Please test again after I updated.

@infograf768
Copy link
Member Author

@richard67
OK, you can test again now.
The modifications concern the version in RTL (Test by using Persian), as well as showing the Joomlaupdate Quickicon in Green when there is no Update available.

screen shot 2018-11-18 at 11 40 31

screen shot 2018-11-18 at 11 48 41

@richard67
Copy link
Member

@infograf768 I don't see any difference in the Persian screenshots. What has changed?


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

@infograf768
Copy link
Member Author

formerly (see on top), the version read alpha6-dev etc.
it now displays correctly 4.0.0-alpha etc, i.e. the version is ltr in a rtl lang string

@richard67
Copy link
Member

I see now.


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

@richard67
Copy link
Member

@infograf768 Now I tested and it seemed ok. But after switching back the Joomla update channel to "Default" and changing back Version.php and fixing the database, I get a javascript error from the Joomla update check, and the update check does not finish:
screen shot 2018-11-18 at 11 55 18
Reverting the PR and running "npm ci" does not fix that, so I am not sure if related to this PR or not.


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

@richard67
Copy link
Member

@infograf768 I've started again with clean 4.0-dev, installed patch tester, applied your patch, run "npm ci", switched on system debugging in global settings so uncompressed js is used.
The js error is in jupdatecheck.js, row 32, column 11, which is statement "if (updateInfo.version === options.version) {", and sais "TypeError: updateInfo is undefined".
This seems to come from your last change.


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

@richard67
Copy link
Member

@infograf768 It seems that the "if (updateInfoList.length === 0) {" you've removed with your last change was not there for no reason.

@infograf768
Copy link
Member Author

on ipad now. will look later. the issue was to get the ‘success’ class instead of only ‘pulse’. we may have to define as var and empty the ‘updateinfo.version’ first.

@richard67
Copy link
Member

richard67 commented Nov 18, 2018

@infograf768 Can you check if my PR infograf768#50 against your branch is the right thing to correct it and get what you want with the success class? Unfortunately I made a mistake in title of the commit so it contains "warning" instead of "success", which is misleading. So you better just check it and if ok do the same change yourself.

@richard67
Copy link
Member

@infograf768 I've just tested my changes from infograf768#50, applied to this PR here, and it works. Javascript error is gone, and Joomla Update is green.

@infograf768
Copy link
Member Author

Thanks @richard67
I mark your test Ok again.

@richard67
Copy link
Member

Yes, I've just tested again, works all well.


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

}

// Message specific
joomla-alert .joomla-alert--close, joomla-alert .joomla-alert-button--close {
Copy link
Member

Choose a reason for hiding this comment

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

Alerts are not part of the template. please put RTL CSS in the custom element file

Copy link
Member Author

Choose a reason for hiding this comment

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

@C-Lodder
This is an override for RTL.
The scss for alert is in /node_modules/joomla-ui-custom-elements/src/scss/alert/alert.scss

There is no provisions done for RTL in custom-elements which means we would have to use html[dir=rtl] (which is not a problem I guess).

Also it means that until someone merges a possible pull request in https://github.com/joomla-projects/custom-elements/blob/master/dist/css/joomla-alert.css and possibly https://github.com/joomla-projects/custom-elements/blob/master/dist/css/joomla-alert.min.css, this PR would not correct by itself the RTL display issue with alerts.

EDIT: Maybe it is only in https://github.com/joomla-projects/custom-elements/blob/master/src/scss/alert/alert.scss

The other solution I see is to add these (with html[dir=rtl] ), in the atum/scss/blocks/_alert.scss and also in Cassiopea equivalent file —which means that every 3rd party template will necessarily have to do that to be rtl compatible.

Just let me know what is the way to go.
calling @wilsonge as he has the hand on merging stuff in https://github.com/joomla-projects/custom-elements

Copy link
Member Author

Choose a reason for hiding this comment

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

@C-Lodder
ping 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

After discussion with @wilsonge will make PR towards https://github.com/joomla-projects/custom-elements/blob/master/src/scss/alert/alert.scss
and delete the same in template-rtl.scss>

We will have to wait the PR is merged and compile is done to see the rtl results here.

@infograf768
Copy link
Member Author

RTL can now be tested fully as alert has been updated.
One has to use present 4.0.0-dev branch, patch and run npm ci.

Result:
screen shot 2018-11-23 at 10 12 00

@richard67

@richard67
Copy link
Member

I have tested this item ✅ successfully on 4ed7a1b


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

@infograf768
Copy link
Member Author

@wilsonge
same comment as in the other PR.
RTL testers are a rare category... ;)

@wilsonge wilsonge merged commit 21fd824 into joomla:4.0-dev Nov 28, 2018
@wilsonge wilsonge added this to the Joomla 4.0 milestone Nov 28, 2018
@wilsonge
Copy link
Contributor

Thanks JM!

@infograf768 infograf768 deleted the 4.0-quickicon-all-updates branch November 28, 2018 11:45
@infograf768
Copy link
Member Author

and @richard67 ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
J4 Issue Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants