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

Redesign carousel implementation to reflect documentation - bug fix #44488

Open
wants to merge 6 commits into
base: 5.2-dev
Choose a base branch
from

Conversation

iteidrm
Copy link

@iteidrm iteidrm commented Nov 20, 2024

Summary of Changes

This pull request fixes a bug in the Bootstrap Caraousel implementation of Joomla! (PHP and ES/JS) that causes valid parameters to be removed from the JavaScript options object that is fed to the Carousel instance. It replaces #38930.

Testing Instructions

I (still) do not know if the Patch Tester extension will do the JavaScript re-build / pre-processing for you, and hope you know best what to do in this regard. In any case, I've attached not only a demo / test module but also the changed files.

  • Activate the debug mode in the global configuration to better be able to inspect the generated JavaScript option object in the source code (Browser: Ctrl + U).
  • Install, activate and publish the demo module provided with the pull request on a module position of your choice (e.g. Cassiopeia: main-top). Make sure to set the Menu / Module Assignent to "On all pages".
  • Open / refresh the page and change some params in the module manager (e.g. keyboard | wrap to false).

Actual result BEFORE applying this Pull Request

All options set to false are filtered out, causing the carousel to fall back to its default values (not sliding).

Expected result AFTER applying this Pull Request

Params that can have a false value are recognised and the carousel works as expected.

Link to documentations

@Quy Please let me know if I have missed anything. Also, the problem with filtered parameters may not only exist in the carousel implementation, but in any other Bootstrap component where "false" is a valid option and array_filter is used in the corresponding helper method.

mod_bs_carousel.zip
patched_files.zip

Remove the re-setting of the options, as it is already done in the PHP helper as well as in the bootstrap library itself.
Refactor parameter handling to fix unwanted filtering of valid options.
The ride option can not only be set to "false" or "carousel" but also to "true".
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.2-dev labels Nov 20, 2024
@fgsw
Copy link

fgsw commented Nov 20, 2024

I (still) do not know if the Patch Tester extension will do the JavaScript re-build / pre-processing for you, and hope you know best what to do in this regard.

The Pull Request have the Label NPM Resource Changed; hovering the Label give This Pull Request can't be tested by Patchtester.

@iteidrm
Copy link
Author

iteidrm commented Nov 20, 2024

The Pull Request have the Label NPM Resource Changed; hovering the Label give This Pull Request can't be tested by Patchtester.

@fgsw thanks for the clarification.

@richard67
Copy link
Member

People can download a package created by Drone, or update to that package using a custom URL. Both can be found by following the "Details" link at the right side of the "Download" step in the CI checks at the bottom of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.2-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants