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

Updated Bootstrap 4.5.3 and Popper 1.16.1 #45

Closed
wants to merge 2 commits into from
Closed

Updated Bootstrap 4.5.3 and Popper 1.16.1 #45

wants to merge 2 commits into from

Conversation

malberts
Copy link
Contributor

@malberts malberts commented Jan 6, 2021

As per our discussion in #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 (twbs/bootstrap#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
Copy link
Contributor

s7eph4n commented Jan 6, 2021

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
Copy link

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
Copy link
Contributor

s7eph4n commented Jan 7, 2021

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

@JeroenDeDauw
Copy link
Member

JeroenDeDauw commented Jan 18, 2021

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
Copy link
Contributor Author

@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
Copy link
Member

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 malberts mentioned this pull request Apr 6, 2021
@malberts
Copy link
Contributor Author

malberts commented Apr 6, 2021

Closing this PR because it is superseded by #47

@malberts malberts closed this Apr 6, 2021
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.

4 participants