-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Rid us of id crm-container #31864
base: master
Are you sure you want to change the base?
Rid us of id crm-container #31864
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
@artfulrobot
So IMO it's useful to keep the |
I agree with both the needs: (a) something to apply CRM styles within, (b) something to locate the specific, main page container. I just disagree that this should be the same token "crm-container". Because it's confusing and leads to devs inc themers not knowing which to use. In a project that wants to encourage lots of input, we need this clarity. I suppose "class=crm-styling", id="crm-page-container" or such might be two useful names. Changing all those crm-container classes is hard and of course extensions/themes will also be affected. So let's leave class=crm-container alone. id=crm-container should be much easier to change, for the benefits outlined above. |
Replacement for #27805
These commits focus on what should be non-changing changes to CSS, tpls and JS.
Note that this PR does not remove
id="crm-container"
from templates, but where it was found, it does ensure that there's acrm-container
class as well. This should mean that all CSS/JS still works.There's more to do, but I stopped here to see if there's much interest. Some of the remaining bits make the assumption that
#crm-container
is a single element, which it should be, however.crm-container
I have a feeling may be found nested, on occasion. I could be wrong, but I'm sure I've seen that somewhere. This gives problems for Jquery based code that does things like$('.crm-container').append(somethingWeNeedOnlyOneOf)
since then it might append 2+ - e.g. the menu.We could solve that by
.crm-main-page-container
. Or a similarly named ID.As a reminder of why all this needs to change: too much CSS relied on the
#crm-container
class when a.crm-container
would have done, leading to increased specificity which in turn is harder to override in themes. Also, a mix of rules for no reason. Also, the div#crm-container element does not always contain all the HTML generated by Civi - e.g. jQuery modals etc - which led to theming woes.@vingle