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] Finish transition from CSS classes "label-" to "alert-" for the pre-update check #34227

Merged
merged 5 commits into from
May 29, 2021

Conversation

richard67
Copy link
Member

@richard67 richard67 commented May 26, 2021

Pull Request for Issue # .

Summary of Changes

With pull request (PR) #33393 , the CSS classes used for colouring the legends of the different fieldsets of the extensions compatibility checks depending on the checks' results have been adapted to Boostrap 5.

This PR here completes that by applying the same changes to the legends of the two PHP checks, "Required PHP & Database Settings" and "Recommended PHP Settings" and by changing the CSS class at one place (line 31) where it was forgotten with PR #33393 .

In addition, it moves the alert CSS classes from the legend to the h3 element so the colours are applied by the (existing and not changed) CSS.

Testing Instructions

  1. Code review.

  2. Check if the legends "Required PHP & Database Settings" and "Recommended PHP Settings" are coloured according to the status of the checks.

The easiest way to get an error in the "Required PHP & Database Settings" is to create an empty SQL update script with a higher schema version than the database schema, e.g. "4.0.0-2021-05-26.sql" in the update SQL script folder for your database type, so the database schema checker will show an error.

Actual result BEFORE applying this Pull Request

The legends "Required PHP & Database Settings" and "Recommended PHP Settings" have old BS 2 CSS classes "legend-..." and so are not coloured depending on the check result:

pr-34227_before-with-php-req-error

Expected result AFTER applying this Pull Request

The legends "Required PHP & Database Settings" and "Recommended PHP Settings" have BS 5 CSS classes "alert-..." and so are coloured depending on the check result:

  • "Required PHP & Database Settings" has "alert-success" if all checks have passed and "alert-danger" if not.
  • "Recommended PHP Settings" has "alert-success" if all checks have passed and "alert-warning" if not.

pr-34227_after-with-php-req-ok

pr-34227_after-with-php-req-error

Documentation Changes Required

None.

@richard67 richard67 marked this pull request as ready for review May 26, 2021 10:12
@richard67 richard67 changed the title [4.0] [WiP] Finish transition from CSS classes "label-" to "alert-" for the pre-update check [4.0] Finish transition from CSS classes "label-" to "alert-" for the pre-update check May 26, 2021
@richard67
Copy link
Member Author

@brianteeman Could you test this PR here? Or at least review? Thanks in advance.

@brianteeman
Copy link
Contributor

will do later this afternoon. meetings - yuk

@brianteeman
Copy link
Contributor

This looks pretty ugly

image

@richard67
Copy link
Member Author

@brianteeman Agree, but the ugly look isn't caused by this PR, is it?

@brianteeman
Copy link
Contributor

Technically what is the logic of the two "warnings" getting different colors?

Design wise as you can see from the screenshots the borders appear to be different thicknesses or even invisible

The text should be vertically centre aligned so that its not butting up against the top border

@brianteeman
Copy link
Contributor

Personally I would just remove the class altogether from the legend

@richard67
Copy link
Member Author

@brianteeman Maybe you should check it on 3.10 to see it with the right colours. The left side are required settings so the warning is red because it should stop from making the update, and the right one are recommended PHP settings which won't make the update fail. This PR here corrects the class names in the same way as you have done it before with PR #33393 , nothing more, nothing less.

@brianteeman
Copy link
Contributor

I can see where the problem is. give me 5 min

@brianteeman
Copy link
Contributor

The suggestions I just made will produce

image

The only thing I am not sure about is if you are even allowed to have an h3 inside a legend

@richard67
Copy link
Member Author

@brianteeman I would prefer to remove the fieldset (I do not see any input fields to be grouped here) and use divs. But such change should be made in 3.10-dev, too, and if we do it here and there or only here, it will cause merge conflicts.

@brianteeman
Copy link
Contributor

Sorry I didn't even realise it was a fieldset without fields. That's a massive failure.

@brianteeman
Copy link
Contributor

@chmst this is failing accessibility

@richard67
Copy link
Member Author

@brianteeman I agree, but as said: Shouldn't we fix that in 3.10-dev, too?

@richard67
Copy link
Member Author

@brianteeman So I have implemented your suggestions for having the right colours. Anything else, especially removing the wrong fieldsets for accessibility, is not related to this PR. If you prefer to close this PR and keep the ugly look of the pre-update checker's PHP checks in J4, let me know. But I will not let this PR be bloated up to fixing all issues of the pre-update checker.

@brianteeman
Copy link
Contributor

thats fine - small steps is the way to go.

@richard67
Copy link
Member Author

Then test please.

@brianteeman
Copy link
Contributor

as the code is basically mine now I shouldnt have my test counted

@richard67
Copy link
Member Author

You only have moved it to the h3, but it was still me who fixed the wrong class names.

@Quy
Copy link
Contributor

Quy commented May 26, 2021

Please apply the same move on line 190.

@richard67
Copy link
Member Author

Please apply the same move on line 190.

@Quy Done, even if it doesn't make any visual difference.

Maybe I should stop to clean up after other people's incomplete work to avoid such time consuming discussions in future.

@Quy
Copy link
Contributor

Quy commented May 26, 2021

I have tested this item ✅ successfully on d0151bd

Actually there is a visual difference of .5rem bottom margin. It is much better now. Thank you!

Yes, it can be time consuming but making J4 better!


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

@richard67
Copy link
Member Author

@Quy Well, you are testing at least, in opposite to others who only open issues and expert their PR's to be tested, so honestly thanks for testing.

@chmst
Copy link
Contributor

chmst commented May 26, 2021

I have tested this item ✅ successfully on d0151bd

Works as described. As already mentioned, fieldsets and the missing external link icon are still open.


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

@chmst
Copy link
Contributor

chmst commented May 26, 2021

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 26, 2021
@chmst chmst added this to the Joomla 4.0 milestone May 26, 2021
@rdeutz rdeutz merged commit dd32585 into joomla:4.0-dev May 29, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 29, 2021
@richard67 richard67 deleted the 4.0-dev-pre-update-check-fixes-1 branch May 29, 2021 08:24
@richard67
Copy link
Member Author

Thanks all.

bembelimen pushed a commit that referenced this pull request Nov 23, 2021
* [4.0] Notification icons (#34226)

* remove method allow parent to run (#34118)

Co-authored-by: Richard Fath <[email protected]>

* [4.0] Changing title to tooltip for template preview (#33292)

* [4.0] Changing title to tooltip fpr template preview

* Update location path in description (#34238)

* Fix patterns field check when field empty and not required (#34124)

* [4.0] Finish transition from CSS classes "label-" to "alert-" for the pre-update check (#34227)

* [4.0] mod_popular with disabled hits (#34257)

* [4.0] mod_popular with disabled hits

When hits are disabled for articles it makes no sense to display the Most Read Articles module in the site or admin as the contents will never be updated.

This PR changes the output of the module so that instead of a list of non-updating articles a message is displayed.

* [4.0] Use MVCFactory to create model (#34092)

* Use MVCFactory to create models

* CS

* Fix row selecting, when module is disabled (#34273)

* Media web service implementation.

* Media web service implementation.

* Fix hard coded adapter name default.

* Fix missing url and tempUrl attributes in single item response.

* Remove useless comments.

* Replace 'PATH' with 'STRING' for cleaning of input parameters.

* Adds required parameter checks and removes global exception handling.

* Adds media web service specific language strings.

* Adds exception handling.

* Adds missing exception handler and fixes wrong path return value after move/rename.

* Use exception message as error title.

* Adds proper FileNotFound exception handling for GET requests.

* Fixes handling of status code.

* Fixes some types, formatting and similar stuff.

* Fixes yet another sloppy typo.

* restore file

* Fix some docs

* Adapter endpoint

* Add plugin to install file

* Add tests

* Update api/components/com_media/src/Helper/MediaHelper.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Helper/MediaHelper.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Model/MediaModel.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Model/MediumModel.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/View/Media/JsonapiView.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update libraries/src/Error/JsonApi/SaveExceptionHandler.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Controller/MediaController.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Controller/MediaController.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Model/AdaptersModel.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Model/MediaModel.php

Co-authored-by: Phil E. Taylor <[email protected]>

* docs

* Update api/components/com_media/src/View/Media/JsonapiView.php

Co-authored-by: Phil E. Taylor <[email protected]>

* int

* cs

* cs

* more cleanup

* tabs

* restore

* Test all endpoints

* Update api/components/com_media/src/Model/MediaModel.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Hardening

* Tests fix for drone

* Update Api.php

* Create directory in test with correct permissions

* Add more tests and remove static helper

* Update tests/Codeception/api/com_media/MediaCest.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update tests/Codeception/_support/Helper/Api.php

Co-authored-by: Phil E. Taylor <[email protected]>

* cs

Co-authored-by: Brian Teeman <[email protected]>
Co-authored-by: Phil E. Taylor <[email protected]>
Co-authored-by: Richard Fath <[email protected]>
Co-authored-by: infograf768 <[email protected]>
Co-authored-by: Quy <[email protected]>
Co-authored-by: Ruud <[email protected]>
Co-authored-by: Tuan Pham Ngoc <[email protected]>
Co-authored-by: Fedir Zinchuk <[email protected]>
Co-authored-by: Pieter-Jan de Vries <[email protected]>
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.

6 participants