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

[5.2] Redesign carousel implementation to reflect documentation - bug fix #38930

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

Conversation

iteidrm
Copy link

@iteidrm iteidrm commented Oct 10, 2022

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.

Testing Instructions

I 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.

  • 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.

Expected result AFTER applying this Pull Request

Params that can have a false value are recognised and work as expected.

Link to documentations

Please select:

Demo Module

mod_bs_carousel.zip

- Fix slide vs. ride attribute in both doc block and code (https://getbootstrap.com/docs/5.2/components/carousel/#options)
- Include missing params in doc block
- Refactor parameter handling to fix unwanted behavior
- Remove parameter cleanup to prevent fallback to default values
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels Oct 10, 2022
@Hackwar Hackwar added the bug label Apr 6, 2023
@HLeithner HLeithner changed the base branch from 4.2-dev to 4.3-dev May 2, 2023 16:29
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 4.3-dev.

@obuisard obuisard changed the base branch from 4.3-dev to 5.0-dev July 15, 2023 18:35
@Hackwar Hackwar added the PBF Pizza, Bugs and Fun label Aug 25, 2023
@ceford
Copy link
Contributor

ceford commented Sep 15, 2023

I have tested this item ✅ successfully on 780d3d8

Quite tricky to test - I had to edit in some of my own image links into the carousel because those supplied are broken. I can turn on each behaviour in turn and see that they work. I actually learned what some of the terms mean!


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

@ceford
Copy link
Contributor

ceford commented Sep 15, 2023

To compile the js:

npm run build:js


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

@ceford
Copy link
Contributor

ceford commented Sep 15, 2023

Ah - I tested this against 5.0.0-beta2-dev


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

@HLeithner
Copy link
Member

@Fedik @dgrammatiko I relabeled this a feature and moved it to 5.1 is this something we want and need?

@Fedik
Copy link
Member

Fedik commented Oct 5, 2023

Looks fine, if it works :)

@dgrammatiko
Copy link
Contributor

Ditto

@Quy Quy added PR-5.1-dev and removed PR-5.0-dev labels Oct 6, 2023
@HLeithner HLeithner changed the base branch from 5.1-dev to 5.2-dev April 24, 2024 09:09
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.2-dev.

@HLeithner HLeithner changed the title Redesign carousel implementation to reflect documentation - bug fix [5.2] Redesign carousel implementation to reflect documentation - bug fix Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature NPM Resource Changed This Pull Request can't be tested by Patchtester PBF Pizza, Bugs and Fun PR-5.1-dev PR-5.2-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants