search

ProfessionalWiki / Bootstrap

Provides the Bootstrap 4 web front-end framework to MediaWiki skins and extensions
https://www.mediawiki.org/wiki/Extension:Bootstrap
Other
14 stars 13 forks source link

Updated Bootstrap 4.5.3 and Popper 1.16.1 #45

Closed malberts closed 3 years ago

malberts commented 3 years ago

As per our discussion in https://github.com/ProfessionalWiki/Bootstrap/issues/44

I updated Bootstrap 4 and Popper 1 to their latest versions. Popper 2 doesn't look like it is going to be supported in Bootstrap 4 (https://github.com/twbs/bootstrap/issues/29842).

I used npm 7.3 to update the libraries. I noticed the Javascript source maps are not getting copied over (due to the install command copying only .js), but they used to be in the folder. Were those files copied over manually before, or should I update the install command to also copy .js.map?

I am not sure what would constitute sufficient testing for this. The side-effects of this update will be more visible on the Chameleon skin, but any changes there would have to be a separate PR. I haven't picked up any specific issues there yet, but I did only very basic checks to see if the layout and the navbar popup are still working (MW 1.35.1 with Chameleon 3.1.0).

s7eph4n commented 3 years ago

I am not sure how I updated it last time. Could be I did it manually. Anyway, the map files can be useful for debugging, but should not be needed in production anyway, so it might make sense to just delete them.

For what regards testing, this extension really just provides Bootstrap to MW as is, so I think it would indeed have to be checked in e.g. Chameleon (or whoever uses it) if all styles are still working as expected. The tests included here in the Bootstrap extension just ensure that nothing breaks on the server. What they do not ensure is that any new components that might have been added in Bootstrap 4.5 are included. For this you'd need to manually check the components against the list in https://github.com/ProfessionalWiki/Bootstrap/blob/master/src/Definition/V4ModuleDefinition.php. Might be worth to come up with a test for that, e.g. ensure that all Bootstrap SCSS files are accounted for in the $moduleDescriptions, but that's out of scope for this PR.

DesignerThan95 commented 3 years ago

I just read through the issue and pr. Maybe it would be an idea to make it selectable? But I am to bad at PHP so I am not able to implement it.

s7eph4n commented 3 years ago

I don't think that is necessary. If you want an older version, just install an older version.

JeroenDeDauw commented 3 years ago

I don't think that is necessary. If you want an older version, just install an older version.

Agree

@malberts many thanks for the PR. Are you able to test this new version together with the Chameleon skin? If that combination works fine we can merge your PR. I'm rather short on time at the moment so cannot test myself.

malberts commented 3 years ago

@JeroenDeDauw I do plan on using it with Chameleon, but I haven't had much time since this PR. I also started looking at a unit test for this extension, as suggested by s7eph4n above. This is only for a hobby project and it's been a while since I used PHP, so it's taking me a bit longer now, but I will keep you updated once I've done some more testing.

JeroenDeDauw commented 3 years ago

Oops, looks like I forgot to reply here, even though I certainly thought about it.

Automated tests would indeed be fantastic, but are not required. A quick manual test that the skin is not broken when using these new versions will already do.

malberts commented 3 years ago

Closing this PR because it is superseded by https://github.com/ProfessionalWiki/Bootstrap/pull/47