[#34227] - [4.0] Finish transition from CSS classes "label-" to "alert-" for the pre-update check
- Fixed in Code Base
- 29 May 2021
- Medium
- Build: 4.0-dev
- # 34227
- Diff
- richard67:4.0-dev-pre-update-check-fixes-1
User tests: Successful: Unsuccessful:
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
-
Code review.
-
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:
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.
Documentation Changes Required
None.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_joomlaupdate |
| Title |
|
||||||
will do later this afternoon. meetings - yuk
This looks pretty ugly
@brianteeman Agree, but the ugly look isn't caused by this PR, is it?
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
Personally I would just remove the class altogether from the legend
@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.
I can see where the problem is. give me 5 min
The suggestions I just made will produce
The only thing I am not sure about is if you are even allowed to have an h3 inside a legend
@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.
Sorry I didn't even realise it was a fieldset without fields. That's a massive failure.
| Labels |
Added:
?
|
||
@brianteeman I agree, but as said: Shouldn't we fix that in 3.10-dev, too?
@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.
thats fine - small steps is the way to go.
Then test please.
as the code is basically mine now I shouldnt have my test counted
You only have moved it to the h3, but it was still me who fixed the wrong class names.
Please apply the same move on line 190.
I have tested this item
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.
I have tested this item
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.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34227.
| Labels |
Added:
?
|
||
rdeutz
- | Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-05-29 08:02:26 |
| Closed_By | ⇒ | rdeutz | |
| Labels |
Added:
?
Removed: ? |
||
Thanks all.
@brianteeman Could you test this PR here? Or at least review? Thanks in advance.