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

Bootstrap 5 Further Progress #992

Merged
merged 36 commits into from
Jan 26, 2024
Merged

Conversation

liamkeily
Copy link
Collaborator

@liamkeily liamkeily commented Jan 17, 2024

This PR carries on from #988 and #973 to take this a bit further.

Merge request checklist

  • I read the guidelines for contributing
  • I created my branch from dev and I am issuing the PR to dev
  • I didn't pushed the dist directory
  • If it's a new feature, I added the necessary unit tests
  • If it's a new language, I filled the __locale and __author fields

Screenshots Before

image image

Screenshots After

image image

@liamkeily
Copy link
Collaborator Author

@mistic100 is the CI here failing for a known reason?

@mistic100
Copy link
Owner

mistic100 commented Jan 17, 2024

it says it cannot get bootswatch-dist, but this package is for Bootstrap 3 anyway, you probably should remove it

@liamkeily
Copy link
Collaborator Author

@mistic100 yea i've picked up from where the others left off, it is a bit awkward so a bootstrap-5 branch that we can all commit to could be useful

This was referenced Jan 17, 2024
@liamkeily
Copy link
Collaborator Author

liamkeily commented Jan 17, 2024

This branch feels like its in a fairly good place now, although needs further testing.

bootstrap-select does not appear to work for Boostrap 5 so i have commented it out for now. chosen-selectpicker appears to act as an alternative and does work but does not really fit the Bootstrap 5 styling so i have left it disabled for now. The query builder appears to work nicely without either so I'm not sure how much functionality we are missing without it.

Feedback / testing would be appreciated from @stephencmorton @dethell if you get any time. I will also test bringing this branch into the project i use it for to test it further.

@dethell
Copy link

dethell commented Jan 17, 2024

@liamkeily I'm fine with the work continuing here, although if @mistic100 want's us to be committers to a specific branch that's fine too. Whichever works. I'll go ahead and pull this branch and retest.

@dethell
Copy link

dethell commented Jan 17, 2024

This looks good. Checkboxes work, dark mode works, selects work. I can't see any reason not to merge this. Back to you @mistic100 .

@liamkeily
Copy link
Collaborator Author

Thanks for reviewing @dethell

One thing i've wondered is how this will play with the documentation site at https://querybuilder.js.org/demo.html which still uses Bootstrap 3.

@stephencmorton
Copy link
Contributor

I'm fine with any branch as well. If this one is a superset of all the others, I'm good with that.

@stephencmorton
Copy link
Contributor

Oh, the "SRBuilds" id on some of my commits was just me accidentally logged in as my work account. I thought about it and it's ok, I do not need to "git commit --amend --reset-author" or anything like that, my work would be fine with it and it does not reveal I should not. I actually started this because I investigated using jquery-querybuilder for an internal tool.

@liamkeily
Copy link
Collaborator Author

liamkeily commented Jan 19, 2024

@mistic100 Do you have any information about how this project is built and published to npm? I wanted to replicate it as closely as possible to make sure it all works but i'm a bit confused by the setup.

I just noticed that the dev branch package.json still contains an older version "2.5.0" than the latest release "2.7.0".

@mistic100
Copy link
Owner

That's because the release is done from the master branch.
release

To do a release :

  • merge dev into master
  • change version in package.json
  • run npm exec build
  • amend the merge commit
  • run npm publish

@mistic100
Copy link
Owner

If I had to really work on this lib (I won't) I would reuse the workflows I created for my other library https://github.com/mistic100/Photo-Sphere-Viewer/tree/main/.github/workflows

@liamkeily
Copy link
Collaborator Author

I replicated the release process and have been testing this on a real project and fixed a few more minor bugs. It's shaping well, just want to do a bit more testing then might be time to start thinking about tagging up a 3.0.0 pre-release.

What do you think @mistic100 ?

@stephencmorton
Copy link
Contributor

Thanks so much @liamkeily . Yeoman's work here.
It looks like you removed selectpicker and then re-added it. For a clearer history would it be possible to remove those commits, or if there's not a net-null result, at least squash them into whatever the net change is.

@liamkeily
Copy link
Collaborator Author

@stephencmorton yea i'll get rid of those commits for now. I removed it thinking we could just drop it, but then i realised that the demo itself relies on bootstrap-select for language selection.

@stephencmorton
Copy link
Contributor

(We could just change the demo, if that makes the most sense.)

@mistic100
Copy link
Owner

Go ahead, squash merge the PR into dev and I'll publish a pre-release to npmjs.org

@liamkeily
Copy link
Collaborator Author

liamkeily commented Jan 23, 2024

(We could just change the demo, if that makes the most sense.)

I've gone ahead and removed the bootstrap-select from all places. Although it may be a nice to have for some users, It doesn't appear to be an essential part of the builder and i'm struggling to get it to work correctly. The chosen-selectpicker plugin is also still available for anybody that wants select box filtering functionality.

I've also updated screenshots above

@liamkeily liamkeily merged commit af9e133 into mistic100:dev Jan 26, 2024
1 check passed
@liamkeily liamkeily deleted the dev-bootstrap-5-liam branch January 26, 2024 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants