Add search extension #17775
Add search extension #17775
Conversation
|
(Standard links)
|
|
I updated and re-ran civix, but there are still style errors in the generated files :( |
|
This is the search functionality eventually intended to replace the impossible-to-maintain code in the query builder class. This PR adds it to core but does not take action to enable it - the expectation is that it will retain it's extension structure / separation but eventually become required so that we can leverage it to help replace other hard-to-maintain pieces of code that currently leverage the query object. However, that is down the track and we certainly expect to continue to maintain advanced search./ civireport etc for quite some time - at least one year if not 2 or more after which we would hope to stop accepting new features (fields, filters etc) and only fix bugs in it. Once merged the next steps will be
I'm going to post a blog in the near future just explaining that we are starting to move extensions into the core codebase where
(note something like flexmailer hits the first 3 but not the 4th of these but it is very very clearly intended to replace bad code per number one - so perhaps number 4 is a weighting, not a requirement....) |
|
@colemanw just in regards to unit testing on this are there any actual unit tests in the extension or is it all in the apiv4 unit tests? |
|
@seamuslee001 all the tests are in core. |
|
@colemanw & only on the php /api side? |
|
Right. I haven't added js tests yet. |
|
ok - would be good to do at least some fairly quickly as they need to be configured into our tests too |
|
I added the has-test because of all the tests added on the php / apiv4 side -(previously) but comments above still stand |
|
@eileenmcnaughton I just tried this and saw:
Is this really a requirement? |
|
They way it's currently implemented, yes some of the functional markup relies on bootstrap. |
|
:( I was just trying to walk through and it was tough. Then I saw the notice. My issue is Shoreditch is so invasive on non-drupal sites that it borders on unusable. We haven't used shoreditchon a production site and seeing new functionality rely on it is disappointing |
|
@kcristiano so it should be 'bootstrap' not shoreditch - but we definitely need to solve the dependency. I feel core needs to ship with some sort of optional bootstrap theme |
|
@eileenmcnaughton I agree. Even an optional css library that extensions can call as opposed to the Shoreditch extension. |
|
So I tried it with the basic-out-of-the-box bootstrap 4 (https://github.com/eileenmcnaughton/bootstrap )
And with 'raw' bootstrap 3 And with shoreditch I then wondered how much of the css that is in the extension is in service of shoreditch so I removed it and got Bootstrap 3
My personal preference would be to ship the 'raw' bootstrap 4 as a default bootstrap theme and have enough css in this file to make that work and put anything that supports shoreditch in the shoreditch theme |
|
It seems that disabling font size (in civicrm.css) made the boxes fit... If this works with the default bootstrap then it will likely be 'ok-ish' on any bootstrap |
|
The apiv4 explorer also becomes usable in pure bootstrap with the font size removed and the buttons 'fixed' - although it might look better. I feel like shipping the bootstrap 4 (disabled) and collectively work on the minimum tweaks to get it to 'acceptable' on these 2 pages makes sense as a way to work towards having an out-of-the-box way to have bootstrap working. I think we should focus on the newer version since we 'have to do the work' & other themers will be writing in v4. Shipping enabled for these 2 components can wait until they are at least an MVP |
|
@eileenmcnaughton Thank You for spending time on this. I Agree we can wait a bit. For those of us already using alternate admin themes that are not boostrap based I was thinking we could perhaps register bootstrap just for the components that need it (APiv4, Case V2, Search) We use the WP theme by @christianwach bundled in CiviCRM Admin Utilities on all our sites and when I use Joomla I use one of @vingle themes. I'd rather not have to remove these, but that is a discussion for another day. |
|
I'm reading thru this PR and trying to figure out, other than buttons, and a couple of checkboxes, what Bootstrap is really being used for here? There's also some quite targetted custom css positioning added in search.css as well as some generic flexbox classes which maybe would sit better in civicrm.css or elsewhere as they could be used by other elements outside of search. Sorry if this is too much of a broad question for here - but would it not be easier to take the button-specific parts of Bootstrap 3 / 4 css, put them in a buttons.css and then rather than having to add 47kb+ in aditional file load for Bootstraap, it would be under 3-5kb? Shoreditch and other Bootstrap themes' (?) styling would still apply on those buttons, but they'd also look good natively cross-CMS, and we could start updating all of Civi's buttons to a consistent syntax without requriing the full Bootstrap library for it to work? And any of us who've made a cross-CMS Civi theme (https://github.com/vingle/st.visuali.finsburypark), could also update it to target those selectors. |
|
Just seen this... and am astonished. There is no way that Shoreditch (which is only really fit-for-purpose on Drupal) should be a dependency of Core. I hope that's just crazy talk and not a plan. It would seriously impact those of us providing alternative themes for CiviCRM. FWIW I agree 100% with @vingle on this - i.e. extract what's needed from Bootstrap by default - and let themers theme from there. |
|
@mattwire I am not astonished nor upset this was merged in. In fact I think that allowed a brighter light to be shown on this. I see this new functionality as a game changer. As long as we have time to discuss before it's enabled by default and the new defacto search I am OK with that. I certainly have issues on dependencies and how we choose them. I think we must support out of the box the theme we ship with. I can't install any css theme from in-app and we know that shoreditch needs a manual install. @jamienovick Sure I can go through and document what is causing issues in WP. I'd liuke shoreditch to work on allm CMS. I'd prefer that we have the ability to theme CiviCRM and not have a hard dependency on a single theme. I agree with @vingle that the new features (thinking of API v4, Search and Case v2) should ship with any css they require and allow themers to tweak as needed. I do like this leap forward, I really do like the discussion and attention and expect this will be a giant improvement for CiviCRM. |
|
Hey @jamienovick – I think that'd be a great discussion to have, although separate from this issue, where I'm in agreement with @christianwach & @mattwire. My experience of LexIM working well with Civi was the KAM menu where there was a long process of discussion and testing before final merge into core; creating a dependency on Bootstrap seems a big change. Before I went down the path of trying to make a Wordpress/Drupal7/Drupal8/Joomla-compatible theme I did try and start to debug Shoreditch for Wordpress as I wanted to use your CiviCase. But because Shoreditch compiled to a 1.8mb compressed css file it's not hugely easy (for me at least) to dive in. That was why I tried to keep Finsbury Park's civicrm.css at max 100kb commented & uncompressed. It also became clear while making it that fixes for some CMSs create problems in other CMSs, which is why I used a single css file ('shelford') that four installs of Civi could all read from at the same time, so every change was reflected across each CMS without further syncing/merging/compiling (and still had to create a few CMS-specific selectors for each CMS). But there's probably workflows to make debugging Shoreditch for other CMSs easier – so would be good to know & document/record. There was some discussion a while back somewhere about a UI/UX specific virtual sprint / day – would get my support if there was. |
|
Generally speaking, I'm quite a fan of the merge early & hide behind a feature flag approach (which roughly translates to "hidden extension" in our ecosystem). I do think it would be a good idea to make it hard(er) to find/enable in release builds at this stage - it should be very clear that this not yet intended for end users. With this approach, I feel like any further development has to go through the usual review process and will generally get much more community exposure. This doesn't usually work as well in other repos. (Also, very excited to give this a spin!) |
|
Wee tangential, but on Shoreditch, I'm a fan, but have also run into issues:
I'm sure we can get Bootstrap issues fixed by the time this reaches general availability (enabled by default). To some extent, I'm happy we're being forced to finally address theming issues. |
|
Thanks @bgm I did not know of that repo. I do think we should have a good default theme that supports all our features and allow others to create themes that change the look. I'd really prefer extension specific (or new app specific) css to have it's own css file(s) that we could dequeue and replace as needed by the alternative themes. |
@mlutfy - do you mean get Shoreditch working across all CMSs? Or make CiviCRM's Grenwich theme include Bootstrap? I think Shoreditch is great too – but 1.8mb of compressed CSS for an admin theme isn't so great for settings where bandwidth is at a premium: ie most people on a mobile in at least half of the world – so seems to me like it shouldn't be a dependency for things in core but something you can install. |
|
It was just a tangent. Personally I don't think CiviCRM should depend on Shoreditch. I do think we have to deliver value to users, and we have been running in circles for the past years on CSS issues (edit: to clarify: running in circles on what CSS to ship for a default core install). Right now I think that means having support for Shoreditch, Haystacktheme, Finsburypark or Eileen's Boostrap extension? I hope we can get the new search, api4, formbuilder, etc, to work on any bootstrap theme, and consider a path forward (ex: bootstrap 4 and beyond). |
|
Having read thru the existing civicrm.css and rewritten it this year as my first lockdown activity (and then tried to figure out the difference between what's there and what's in all the other .css files in the css directory) I agree wholeheartedly it's a mess. I feel that part of the mess is because no-one's spelt out and documented a clear plan or rulebook for CSS in Civi. Even really simple stuff - do we set text sizes in pixels, ems or rems? Civicrm uses all three; same for padding and margins. I don't say this as a criticism - I'm sure some of it is 15 years old and CSS has changed a lot in that time (has indeed has my own css writing - I only moved from pxs to rems recently). It also seems talk of a 'bootstrap theme' is shorthand for different things to different people. At the time Bootstrap was chosen for CiviCRM (in 2014!), there wasn't Flexbox (in most browsers) and there wasn't CSS Grid. So part of the reason people added a big library like Bootstrap - to make pages responsive quickly - makes less sense now. We want Civi to be responsive - but on many screens the reason it isn't is because of the use of tables - and adding a 'bootstrap theme' won't change that – what's needed is rewriting existing CiviCRm page templates. When I look at the original Bootstrap for Civi blog post - https://civicrm.org/blog/teja/bootstrap-ui-one-small-step-for-civicrm - it looks great! But I've never seen a Civi/Bootstrap page that looks like that. Instead it's bootstrap classes added to old CiviCRM pages and we get the kind of hybrid Eileen shows in the screengrabs at the top. So if all that's being used of Bootstrap for the most part is some button/button group/alert classes that don't take-up a dozen kbs, is it worth requiring people install a theme that includes bootstrap, when (unless I'm missing something) that only can be Shoredtich? |
|
@vingle Sounds like a good bait & switch opportunity. We can't expect Coleman or Eileen to do all that, but api4/form/builder/search all currently use a very small subset of Bootstrap, so there's an opportunity to document those patterns and implement a small extension that provides CSS for those patterns (and then ship that extension with core). If folks agree, I'd be happy to work on that too. |
|
@mlufty – that would have the advantage of revealing that subset so other extensions could use them. I'm guessing there might be a quick way to find all the classes used in api4/formbuilder/search, and then use that as a starting point? |
|
Sounds good. I did a quick grep on afform and search, and extracted these classes: and created an issue here: I'm happy to help, but I'm not really sure where to start. It would be great if, when 'search' is ready to be enabled by default, it didn't depend on Bootstrap. At the same time, I hope css is not a release blocker, so we need to move fairly fast. Would it be worth spinning up a MIH? i.e. let's hire Nic to work on this? (disclosure: I'm participating as a volunteer, not paid by the CT or Symbiotic, I have not discussed this with the CT) |
|
We definitely need to solve the theming issues before we enable this for new sites or even on demo builds. For users this needs to be no different to any other extension at this stage. From a product maintenance point of view this is about where the code lives (& what processes it is under / who maintains it). I see flexmailer, api4 and export (and maybe KAM) as all having all been fairly problematic in implementation because we hit issues moving the code later on and none of the benefit of the extension-first approach came from having them located in different repos |
|
Ok the blog is posted - https://civicrm.org/node/10272 - |
|
I feel that part of the mess is because no-one's spelt out and documented a clear plan or rulebook for CSS in Civi. I couldn't agree more - as a reviewer I have no guidance on this and as an issue the conversation on this thread is probably the most progress we've had in years. The contact layout editor was another victim of this - it only works nicely with shoreditch as far as I can tell & there is no momentum to fix that to improve it's adoption to provide the momentum to fix that. The product maintenance space is the only space where we have people working on a daily and weekly basis so by bringing this problem into the product maintenance space hopefully it can stop being an unsolvable daily blocker and we can find a way to move forward on it in an incremental way |
|
I just discussed this with @mattwire on chat and he suggested we add the hidden tag - which I have put up a PR to do Note the goal of adding that tag is to separate the code-management issues from the feature availability issues - ie it will not be visible to UI users and that seems appropriate until we have worked through the styling issue. My only reservation is the outpouring of good ideas in this thread is really positive momentum and I hope that out-of-sight is not out-of-mind |
Likewise - and I sense lots of us feel the same. CSS seems one area where Civi's do-ocracy has created a legacy of issues that hold back moving forward, and it would be great to try and use this momentum to resolve some of that. But this doesn't seem like something that can be driven by the community alone. |
|
@kcristiano suggested we form some sort of remote sprint - the css seems like a good topic for it. |
|
I also want to comment a bit on do-ocracy in theming. One thing I hit with shoreditch is that the designers of shoreditch have a clear vision for it - and the do-ocracy methodology doesn't seem to cohere very well with respecting one group's design vision. Whatever we do as something that ships with core needs to be community negotiated - which I think is a bit harder in the design space (design by committee) . |
Yes agreed - but hopefully css/etc architecture qs can be easier to find consensus with than design in itself. |
|
I thought I'd drop this here since it's definitely of relevance: it's a quote setting out the style recommendations from the WordPress Mothership (my emphasis added):
This is exactly what I tried to achieve with the theme bundled in CiviCRM Admin Utilities (the one that @mattwire forked for the HaystackTheme Extension) even though it wasn't an official recommendation at that time. For me, it boils down to UX 101 (i.e. "support user expectations") and, in a WordPress context, this means attempting (as far as possible) to make the CiviCRM UI as "native" as possible. I'm okay with CiviCRM settling on Bootstrap (even though it's kinda obsolete as @vingle points out) as a dependency (as long as I'm able to dequeue it) but I'm definitely not okay with Shoreditch and its 1.8 MB CSS as a dependency. This would set a terrible precedent for people who aren't connected via broadband - I personally know folks in rural India who rely on expensive mobile data for their connections and Shoreditch would make CiviCRM practically unusable for them. I strongly support the suggestion by @mattwire for an extension tracked via a separate repository for at least 2-3 versions of CiviCRM. |
|
@christianwach we are not exposing the extension via the UI until we have worked through issues so it's the same from a user POV - it's just where the code sits is different. We are using the hidden tag (per Matt's suggestion) to manage that. Can you take the css conversation to the gitlab issues per #17775 (comment) |
|
Because @colemanw's PR and the subsequent merge added a dependency on Bootstrap css, which has a dependency on Shoreditch, which isn't compatible with around half of Civi instances it'd be nice to hear what his css thinking was/is with this PR? ie was the expectation everyone runs Shoreditch and it gets ported to WP/J!/D8, or we add a flex-css for required Bootstrap classes? If the answer is longer or more nuanced than this thread allows for – could we have a call / micro-summit to resolve the css framework questions once and for all and set a mroe solid/documentable UI direction for the future? |
|
I suggest someone create a PR that adds a "flex CSS" file or extension to core and it'll give us some more meat to discuss. |
|
@vingle I think this PR should be viewed at the level of 'create a new repo' - ie code management. We have created ourselves and others a tonne of work by putting the code in one place and then moving it for other stuff intended to be a core extension/ in core eventually and all the value that achieves is the same as the value of not revealing via the UI. I think the expectation is that the css issue was not yet solved, and needs to be before it can be UI accessible / move to 'stable' |
|
I updated the flex css wiki with some options for how we could implement it. There's some good conversation happening in dev/user-interface#24 regarding framework options, too. @herbdool said:
I worry that would be fairly labor-intensive to do such a file or extension in a substantial enough way to facilitate discussion, and it would also mean that someone would have to go fairly far down one of a handful of possible directions for implementing this. Specifically, the task would differ, varying from completing custom CSS for things that Bootstrap natively handles, to implementing these things in other frameworks, to making everything Bootstrap-friendly. |
|
@agh1 that git lab is well laid out & makes the options fairly comprehensible - kudos |
|
This UI now works for non-shoreditch users (@totten sunk a few weeks into some of the underlying technical issues off the back of this discussion)
|
|
Now that the theming work this triggered has made a bootstrap 3 theme backup available (and fixed a lot of themingg layer contract stuff) there is a PR open to unhide the extension #18672 |
Overview
This PR adds the Search extension to the main CiviCRM package.
Before
Separate extension.
After
Together!