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

IBX-3456 Collapse all button support #1394

Open
wants to merge 12 commits into
base: 4.6
Choose a base branch
from
Open

Conversation

bozatko
Copy link
Contributor

@bozatko bozatko commented Nov 18, 2024

🎫 Issue IBX-3456

Related PRs:

Description:

For QA:

Documentation:

@bozatko bozatko changed the base branch from main to 4.6 November 18, 2024 11:51
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
const collapseAll = toggleAllBtn?.querySelector('.ibexa-attribute-group__toggler-collapse');
const MULTI_COLLAPSE_BODY_CLASS = '.multi-collapse';
let singleElementClicked = [];
const checkIfChangeText = () => {
Copy link
Contributor

@lucasOsti lucasOsti Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name of the function is wrong because this function only toggles the d-none class.
I have one more question, what will happen if it will be 2 or 3 collapse all buttons for different groups?
I would probably create a new JS file that contain all the collapse/expand logic for multiple collpase. The button in the data-attribute could be given a selector which collapse it should collapse/expand/listen to, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I proposed a new name for the function. In my opinion, based on the YAGNI principle, we should first discuss the probability of such a situation before making such changes.

@@ -0,0 +1,37 @@
.ibexa-multi-collapse {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved styles from product-catalog. Based on previous review comment let me know if I still should change class name to ibexa-pc-...

@bozatko bozatko force-pushed the IBX-3456-attribute-groups branch from c1599eb to c43983f Compare December 12, 2024 15:43
@bozatko bozatko force-pushed the IBX-3456-attribute-groups branch from 1228b09 to c8db495 Compare December 13, 2024 10:24
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
src/bundle/Resources/public/js/scripts/core/collapse.js Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Dec 16, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants