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

Content slider CSS class collision #6420

Closed
leofeyer opened this issue Oct 5, 2023 · 11 comments
Closed

Content slider CSS class collision #6420

leofeyer opened this issue Oct 5, 2023 · 11 comments
Labels
Milestone

Comments

@leofeyer
Copy link
Member

leofeyer commented Oct 5, 2023

Affected version(s)

5.0.0+

Description

Our content slider has an inner DIV with the CSS class content-slider:

<div class="content-slider" data-config="<?= $this->config ?>">

Since the new Twig templates use content-{type} as naming schema, we have to adjust the Contao CSS framework accordingly:

But adding .mod_article *[class*="content-"] would also apply the grid styles to the content slider DIV, therefore we should rename the CSS class of the content slider. @contao/developers Any objections?


BTW, we did not stumble upon this earlier, because the slider content element has not been converted to a controller yet. @m-vo Did we leave it out on purpose?

@leofeyer leofeyer added the bug label Oct 5, 2023
@leofeyer leofeyer added this to the 5.2 milestone Oct 5, 2023
@ausi
Copy link
Member

ausi commented Oct 5, 2023

Could we instead use something like .mod_article > * to set these margins?

@leofeyer
Copy link
Member Author

leofeyer commented Oct 6, 2023

No, that would not really solve the class collision issue. It would just be a workaround in the grid.css style sheet, but the problem will likely occur in other places, too.

@m-vo
Copy link
Member

m-vo commented Oct 6, 2023

BTW, we did not stumble upon this earlier, because the slider content element has not been converted to a controller yet. @m-vo Did we leave it out on purpose?

Yes, we left out all content elements that currently use wrappers. The intention was to implement them as 'real' nested content elements as soon that feature has landed. 🙂

@m-vo
Copy link
Member

m-vo commented Oct 6, 2023

What about always adding a class to each content element in the base template? Then we could target content elements directly in the CSS instead of relying on search/prefix? (On that note: Why are we using [class*="ce_"] instead of [class^="ce_"]?)

Too bad we reverted the content element CSS naming in 817e867. 🙈 Otherwise you would have gotten a nice class="content-element content-element/text. But now content-element as a class name would clash with a type named element. Just thinking out loud but you get the point. 😄

@leofeyer
Copy link
Member Author

leofeyer commented Oct 9, 2023

What about always adding a class to each content element in the base template?

That does not solve the name collision issue, either, I‘m afraid. Once the slider element is converted, it would have the content-element CSS class on the outer DIV as well as on the inner one.

@ausi
Copy link
Member

ausi commented Oct 9, 2023

Once the slider element is converted, it would have the content-element CSS class on the outer DIV as well as on the inner one.

The inner classes of the other new twig content elements change too, so I’d guess that we would change the inner class name when converting the slider to a new element controller.

@leofeyer
Copy link
Member Author

leofeyer commented Oct 9, 2023

We cannot change the inner class unless we also adjust the slider JS. That‘s why I suggest to do it right away.

@leofeyer
Copy link
Member Author

leofeyer commented Oct 11, 2023

Unfortunately, #6435 does not fix the issue. 😞

Accordion elements get the class accordion ui-accordion-content … ui-accordion-content-active and [class*="content-"] matches that, too.

Why are we using [class*="ce_"] instead of [class^="ce_"]?

Because [class^="ce_"] would only match class="ce_text" but not class="foo ce_text".

Otherwise you would have gotten a nice class="content-element content-element/text.

I guess that would have prevented this issue. Maybe we should use content-element-{type} instead of content-{type}? Or both content-element and content-{type}? Then we could use a simple class selector .content-element instead of [class*="content-].

@ausi
Copy link
Member

ausi commented Oct 11, 2023

I think we should move away from selctors like [class*="ce_"] they are way too error-prone and can probably be replaced with better selectors. [class*="ce_"] would also mach classes like nice_box and so on.

AFAIK we use [class*="…"] only in the grid.css and nowhere else. So I’d suggest replacing these selectors there.

If we want to be extra careful regarding BC, we could move from [class*="ce_"] to
:is([class^="ce_"], [class*=" ce_"]) as a first step to avoid most collisions, and replacing them with something else in the next version.

@aschempp
Copy link
Member

I think we could also just get rid of the grid.css in an upcoming minor release and have people come up with better solutions for that. 😬

@leofeyer
Copy link
Member Author

Eventually fixed in contao-components/contao#16.

leofeyer added a commit that referenced this issue Dec 29, 2023
Description
-----------

This PR adds a modern content slider element now that we have nested fragments. 🥳 

To prevent naming collisions with the legacy content slider (see #6420), the new element uses a new Contao component: https://swiperjs.com 

Because Swipe.js has been dead for years now, this step was long overdue.

Commits
-------

d441034 Add a modern content slider element
bfc06db Wrap the HTML markup in a separate block
bb55aa0 Rename "swiper" to "slider"
5c7466f Use separate blocks for styles and scripts
a0dbf4f Add a unit test
f8c08e8 Rename the element again to prevent conflicts with Swipe.js
dd526b7 Adjust the tests
a23491e Use <button> elements for the next and prev buttons
b439190 Fix the tests
fdf68f4 Make the slider attributes editable
449b5f4 Fix the tests
cb3e1a0 Add more blocks in the template
fef16d7 Adjust the button attributes
3901c90 Add the `_attributes` suffix where missing

Co-authored-by: ausi <martin@auswoeger.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants