search

mistic100 / jQuery-QueryBuilder

jQuery plugin offering an interface to create complex queries
https://querybuilder.js.org
MIT License
1.67k stars 552 forks source link

Bootstrap5 support #973

Closed stephencmorton closed 5 months ago

stephencmorton commented 1 year ago

I started to work to make jQuery-QueryBuilder work with Boootstrap 5. I believe I've done >80% of the "Bootstrap" work, I just need to verify that the popovers/tooltips work. (I do not think they do but I do suspect that the fix is very simple.)

So far it is pretty easy.

What is hard for me is the "npm" stuff. "packages.json" etc, making all the versions work together. I'm a professional programmer but I do not work in nodejs so all this npm build stuff is foreign to me. I imagine that what an experienced person in this area could finish in 20 minutes I could work on for hours and still fail. I'm hoping that somebody else can pick up that part and quickly do it.

Merge request checklist

mistic100 commented 1 year ago

If you finish the migration I will publish it.

If you are professional programmer I am sure you can follow the build guide :) https://github.com/mistic100/jQuery-QueryBuilder#build

stephencmorton commented 1 year ago

Great, I'll finish the work then. Thanks.

Building is difficult because this uses nodejs-legacy and no amount of coercing will get it to build in a modern Ubuntu environment. I'll try in a legacy nodejs container. (And if you build it with non-legacy nodejs, it results in a sea of errors.)

mistic100 commented 1 year ago

Okay I didn't realize the stack was so outdated.

Unfortunately I have no will to spend time working on this. If the changes are simple enough I will hack into to dist files directly, otherwise....

stephencmorton commented 1 year ago

I can get it to build (npm install; grunt) in a container with an older node version (using node10 or node12 e.g. circleci/node:erbium-bullseye-browsers-legacy from dockerhub) but I cannot yet pass the unit tests grunt test --force on the pristine-without-my-changes dev or master branches. I will keep trying.

(Re-posting same comment as before that I accidentally posted from the wrong account.)

mistic100 commented 1 year ago

Don't try too long, the unit tests are broken since years.

mistic100 commented 1 year ago

I spend this morning reworking the dev tools, not more outdated dev dependencies and it works on Node 16 (probably higher)

mistic100 commented 1 year ago

Beware I removed doT.js dependencie and modified the templates.

stephencmorton commented 12 months ago

I was just rebasing on the latest. This is not 100% yet.

abeverley commented 10 months ago

Hi @stephencmorton - I just wondered how this is going - would some sponsorship help to get it over the line?

stephencmorton commented 10 months ago

Summer holidays and family things mostly. I'll see if I can get back to this. Sponsorship other than getting my job to give me more holiday, or my kids to be less work, is not needed. But thanks.

stephencmorton commented 10 months ago

I've got it working. I submit it for approval/inclusion.

I did have to comment out the bootbox support. It's a NPM/Javascript issue that I assume is simple for somebody who knows more about this area than I do. "module" is not defined .

It's all a bit thorny with all the NPM inclusions and a lot of other modules are still working on their Bootstrap5 support too.

weshooper commented 9 months ago

Thanks for your efforts on this @stephencmorton (and @mistic100!), much appreciated. Also happy to sponsor if there's an option to do so ❤️

abeverley commented 9 months ago

Thanks again for this @stephencmorton - great work.

I'm just working out what (ideally) needs to be done before this is merged (and I may have someone who can help if needed):

  1. Fix the bootbox problem
  2. Make it continue to work with older Bootstrap versions for backwards compatibility (or enable building for both)?
  3. Anything else?
stephencmorton commented 9 months ago

Hi @abeverley,

I think that's about it, other than a general "give it all a good code review and test and fix/improve anything you find". (One definite area too look at is JS packaging: I don't really understand the implications of different packaging methods like commonjs/module/ES6.)

To be honest, I do not see any point in offering backwards compatibility beyond adding to the documentation a section about Bootstrap 3 support being available by using release 2.7.0 and below.

I believe I've taken this as far as my experience/skills will allow so I'd love it if you or @mistic100 could finish any remaining 5% to get it published.

I carefully made separate commits for each "major area" of change, so for instance, if somebody decided that the way I changed the icons was not the desired way forward, it would be easy to redo just that one commit.

mistic100 commented 9 months ago

I agree backward compatibility is not needed. I can publish as V3 but I cannot test it. Do any of you want to be maintainer of the project to be autonomous on the BT5 support ?

liamkeily commented 9 months ago

I have tested this using yarn and yarn run serve

Before

https://github.com/mistic100/jQuery-QueryBuilder/assets/2040842/67a8bada-3e9d-4335-b54c-06de7bf1b519

After

https://github.com/mistic100/jQuery-QueryBuilder/assets/2040842/b7f6cf9a-7234-48bb-aad7-580255ce5ef1

Issues

stephencmorton commented 9 months ago

I'm not sure if it works with yarn anymore. I believe Damien suggested I run npm run build and npm run serve.

(The documentation in this fork was updated to reflect that. https://github.com/stephencmorton/jQuery-QueryBuilder/tree/dev-bootstrap-5#developement )

liamkeily commented 9 months ago

@stephencmorton ah ok, in that case perhaps we need to get rid of yarn.lock and commit a package-lock.json

mistic100 commented 9 months ago

There should not be any difference, NPM and Yarn install there dependencies the same way in the same location. and "npm run" and "yarn run" are strictly identical

but a major dependencies upgrade it might be useful to remove the lock file and recreate it for good

stephencmorton commented 9 months ago

That could very well be. As I've confessed, npm/js is not my area of expertise.

stephencmorton commented 9 months ago

I had pointed out that it was not possible to build and Damien updated some of it to work with a newer Node version. He'd tested to Node 16, I believe so I never tried building on anything higher than 16 either. (Built in a docker.)

stephencmorton commented 8 months ago

I have tested this using yarn and yarn run serve

Issues

* [ ]  When i first cloned this branch i had trouble with `yarn`. I think the `yarn.lock` file is messed up. `git checkout dev yarn.lock` and `yarn` sorts it. The fixed file needs committing to this branch.

* [ ]  The default theme looks a bit broken / not central compared to before

* [ ]  Checkboxes have broken icons  (Still using Glyphicons here https://github.com/stephencmorton/jQuery-QueryBuilder/blob/81175dd6fe7e1ada7e69cabd44268932629c5fd7/src/plugins/bt-checkbox/plugin.scss#L3)

* [ ]  Checkboxes no longer inline

* [ ]  Field select boxes are not being populated when clicking the set data buttons

There are two possible types of errors here

  1. The example index.html file (and its css) needs updating.
  2. There is a bug in the jQuery-QueryBuilder code.

These are quite different types of problem/seriousness.

stephencmorton commented 8 months ago

I welcome anybody to do some last fixes/polishing on this MR. I've done the "heavy lifting" and if anybody has the motivation/energy/skills to find any issues that they feel are blocking and then fix those issues, that would be fantastic. (I would not take it as an insult to my "ownership" of this MR, quite the opposite. I welcome collaboration/help.)

dethell commented 5 months ago

@stephencmorton I started with your branch and have so far added a few changes to fix the checkbox rendering to correctly be inline and using BS5 classes: https://github.com/mistic100/jQuery-QueryBuilder/pull/988 It still looks a bit off, visually, where instead of rendering the SVG checkbox I just get a white square. I'll keep working at this PR and see if I can't address the remaining concerns. I was hoping to contribute my changes to your PR so yours is the one that gets merged and you get the credit for the work.

abeverley commented 5 months ago

Hi @stephencmorton and @dethell - thanks for your work on this, it's much appreciated. Just to mention that I have someone in my team that I should be able to put onto this to help finish things off. Would you like to let me know if/when you need him to jump in?

Also, I was thinking about the backwards compatibility. Whilst I can see that different versions could be used with different versions of Bootstrap, I think it would be useful for the latest version (i.e. with this patch) to still work with Bootstrap 4. That version of Bootstrap is still widely deployed (including by my company) and it would be good to be able to use the latest version of QB to with it. I'm not sure what this would involve but I'd be happy to look at it.

dethell commented 5 months ago

Hi @stephencmorton and @dethell - thanks for your work on this, it's much appreciated. Just to mention that I have someone in my team that I should be able to put onto this to help finish things off. Would you like to let me know if/when you need him to jump in?

Also, I was thinking about the backwards compatibility. Whilst I can see that different versions could be used with different versions of Bootstrap, I think it would be useful for the latest version (i.e. with this patch) to still work with Bootstrap 4. That version of Bootstrap is still widely deployed (including by my company) and it would be good to be able to use the latest version of QB to with it. I'm not sure what this would involve but I'd be happy to look at it.

I'm happy for any help. I don't use Bootstrap 4 anymore, but I could see it being useful. I guess we'd need to evaluate how different it would be to support both.

stephencmorton commented 5 months ago

Thank you both. Yes, I'd appreciate any help. I was imagining that there would be a clean break and 2.7.0 (or 2.7.2 or .4 or whatever) would be the last Bootstrap4 release and then subsequent releases would be Bootstrap5 only. Alternately, I can imagine that @mistic100 could number the first Bootstrap5 release 3.0.0 and then if there were improvements/fixes that needed back-porting to bootstrap4, they could be done in a different feature development branch and releases from that branch could continue with the 2.x.y numbering. Whether or not that actually happens, it does seem to me to make the most sense to have a new release be 3.x.y for that reason and because for semantic versioning this is API-breaking in that it needs a new bootstrap release.

dethell commented 5 months ago

I spent a bit more time today trying to discern why the title in the button associated with each filter select input is not updating with the filter label. I can't see anything in your changes that would cause this so it must be a bug in the interaction between QueryBuilder and Bootstrap 5.

dethell commented 5 months ago

@stephencmorton can you check out the last push I made to my PR based on yours? I removed the use of the bt-selectpicker plugin which causing the select input problem. Now it just defaults to the standard Bootstrap 5 select markup and everything works well. I'm not sure there are any issues remaining except the checkbox icon.

liamkeily commented 5 months ago

I have resolved the checkbox icon in https://github.com/mistic100/jQuery-QueryBuilder/pull/990

stephencmorton commented 5 months ago

Fantastic. Sorry, I meant to test this weekend as promised, but didn't find time. I'll get right on that.

liamkeily commented 5 months ago

I've figured out why Dark mode is messed up in this branch. If you see https://github.com/mistic100/jQuery-QueryBuilder/pull/973/files#diff-a11faa5810ac713931e63304028150455c6e34d4aef559c6ab0b6c7219153ffaR47 when you switch to dark mode it loads in another version of bootstrap from boot-swatch. The version it switches to is Bootstrap 3.

Instead what i think we need to do is load in the same bootstrap 5 file, but set <html data-bs-theme="dark"/> as well as the style overrides from dark.scss.

I'm working on putting together another branch which fixes this and other things mentioned above.

liamkeily commented 5 months ago

Continued at https://github.com/mistic100/jQuery-QueryBuilder/pull/992

liamkeily commented 5 months ago

Closing in favour of https://github.com/mistic100/jQuery-QueryBuilder/pull/992 - thanks for starting this @stephencmorton