ProfessionalWiki / chameleon

Provides a highly flexible and customizable skin using Bootstrap 4
https://www.mediawiki.org/wiki/Skin:Chameleon
Other
114 stars 62 forks source link

Update Grid.php to support container size with mode attribute #324

Closed chrisrishel closed 2 years ago

chrisrishel commented 2 years ago

I think the container or container-fluid class should be added before calling the parent constructor to allow explicitly defined classes in the layout to override the container class (e.g. with something like container-md).

malberts commented 2 years ago

I'm not sure if this has the intended outcome.

When the layout contains: <grid class="container-md"> Before the change the generated classes are: class="container-md container" With this change it flips around: class="container container-md"

The order of the classes in the class attribute has no impact on precedence. That is determined by the CSS order.

Note that the class attribute in the layout XML does not override the actual classes - it adds more classes.

Ideally there should be a way to bypass this logic: https://github.com/ProfessionalWiki/chameleon/blob/78faac87a5a534265c38751b4dbe64f138ce8122/src/Components/Grid.php#L53-L57 Ultimately there should be only one "container[-...]" class on the wrapper.

Unfortunately we cannot use the presence of class in the layout XML, because the current existing behavior is used to add classes, e.g.: https://github.com/ProfessionalWiki/chameleon/blob/78faac87a5a534265c38751b4dbe64f138ce8122/layouts/standard.xml#L26-L29 (Which results in class="flex-fill container")

Two possible solutions that come to mind:

chrisrishel commented 2 years ago

That's interesting. I had always assumed it was the ordering of classes within a class attribute that determined precedent when the selector specificity was otherwise equal, but I guess it was coincident that those later classes also appeared later in the code (which does give higher precedent).

I'd considered expanding the support of the mode attribute but thought it might be avoidable with that first patch. Since not, here's a new patch that adds support for sizing using the mode attribute.

malberts commented 2 years ago

Thanks for the update. I'll update the docs separately.

There is an edge case where this won't work correctly: It is possible to define custom breakpoints (e.g. xxxxl) which won't be considered valid here. However, I don't think this is very common and since this is a new feature we're not actually breaking any existing behavior. If somebody does eventually run into this then we can address it separately.