Skip to content

Conversation

@wjonassen
Copy link
Collaborator

Problem Description

Needed to update the web client with a different theme - went with bootstrap 5.

Fixes #.
Look and feel, as well as a bunch of bugs with the UI in the migration.

Describe your solution.
Tested each page. There are still several issues.

  1. There are several shim files (js/sidebar.js, layout.js, opendcs.css) that are transitional and will not be needed. A separate issue will be set up to move away from these, or merge the functions into the main code base.
  2. Some of the action dropdown datatables in the modals don't function properly. Currently debugging these and will create an issue.
  3. The modals (waiting modals also) appear too high, and underneath the top menu. These need to be pushed down.
  4. Sidebar doesn't quite look right, but it's a start.
  5. Some API calls are not returning successfully, they will need to be debugged.

Each page was tested, each modal was tested, each datatable was tested.

Where the following done:

  • Tests. Check all that apply:
    • Unit tests created or modified that run during ant test.
    • Integration tests created or modified that run during integration testing
      (Formerly called regression tests.)
    • Test procedure descriptions for manual testing
  • Was relevant documentation updated?
  • Were relevant config element (e.g. XML data) updated as appropriate.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 8, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
0.0% Coverage on New Code (required ≥ 30%)
21.1% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copy link
Collaborator

@MikeNeilson MikeNeilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, all of the comments are "brain dump as fast as I can while other work was fresh in brain".

My main concern is the change to the webjars (mostly the npm based one, if we have to use them, sure, but they tend to be oddly packaged.)

You also seem to have some things working that were broken.

However, you have a bunch of needed fixes and tweaks in the js, especially in regards to the datatables and those settings.

So here's my pitch. We either:

  1. Merge this in, then I rebase the reorg branch and we go from there
  2. I rebase the reorg branch onto this and then merge from there.

I don't like the versions in the webjar urls (you clearly didn't either but clearly something wasn't working correctly).

I'm also a bit concerned about reverting the datatables version, but I don't mind doing to work to update that if I'm the one pushing the bit reorg.

My inclination is 1 above, clean merge in of something that mostly works.
And then we quickly follow up with the reorg. Accepting we will likely break a few things again (that PR is already somewhat big) and fix those in follow up PRs.

Considering we basically went in the same direction with bootstrap 5 (and bootstrap-icons (which I didn't see a webjar for so a bit surprised they worked at all)) we can either:

  1. Immediately follow up with the Theme branch changes
  2. Do the fixes and then the theme branch changes.

As much as that branch was just playing around it will force us to behave in a more constrained way when setting up the various components and provide a well defined target for creating say, an "OpenDCS Theme", we just do a bootstrap theme same as how bootswatch does it.

.... Okay that's way too much pontification in a PR review.

Short version. Since you haven't renamed any files, just moved them, it should fairly easy for me to rebase onto this after it's merged.

I would like the reorg in place before we do too much more though. I'm confident it will make it easier for others to come along and certainly makes my brain feel better as I think about all the login tweaks I'm wanting to make.

@MikeNeilson
Copy link
Collaborator

I do like the side bar taking up the full height.

@MikeNeilson
Copy link
Collaborator

I just tested this manually with the docker-compose and it behaves reasonably. There's a few things still broken, but they were before and this isn't the place to fix them. This is a JS/CSS course correction.

I say it's ready to merge, and while my reorg will revert/break some of it, I'm pretty sure it's just the things you called out as shims.

The datatables version reversion concerns me, but that should also be separate from the reorg PR as that will likely require additional code changes.

@wjonassen
Copy link
Collaborator Author

wjonassen commented Oct 8, 2025

Okay, all of the comments are "brain dump as fast as I can while other work was fresh in brain".

My main concern is the change to the webjars (mostly the npm based one, if we have to use them, sure, but they tend to be oddly packaged.)

You also seem to have some things working that were broken.

However, you have a bunch of needed fixes and tweaks in the js, especially in regards to the datatables and those settings.

So here's my pitch. We either:

  1. Merge this in, then I rebase the reorg branch and we go from there
  2. I rebase the reorg branch onto this and then merge from there.

I don't like the versions in the webjar urls (you clearly didn't either but clearly something wasn't working correctly).

I'm also a bit concerned about reverting the datatables version, but I don't mind doing to work to update that if I'm the one pushing the bit reorg.

My inclination is 1 above, clean merge in of something that mostly works. And then we quickly follow up with the reorg. Accepting we will likely break a few things again (that PR is already somewhat big) and fix those in follow up PRs.

Considering we basically went in the same direction with bootstrap 5 (and bootstrap-icons (which I didn't see a webjar for so a bit surprised they worked at all)) we can either:

  1. Immediately follow up with the Theme branch changes
  2. Do the fixes and then the theme branch changes.

As much as that branch was just playing around it will force us to behave in a more constrained way when setting up the various components and provide a well defined target for creating say, an "OpenDCS Theme", we just do a bootstrap theme same as how bootswatch does it.

.... Okay that's way too much pontification in a PR review.

Short version. Since you haven't renamed any files, just moved them, it should fairly easy for me to rebase onto this after it's merged.

I would like the reorg in place before we do too much more though. I'm confident it will make it easier for others to come along and certainly makes my brain feel better as I think about all the login tweaks I'm wanting to make.
@MikeNeilson
Sounds good - let's go with option 1 -
I can take the datatables version update once the merge goes through. I was messing with the newer datatables for a while and I don't think it will be as bad as it seems.

@MikeNeilson MikeNeilson marked this pull request as ready for review October 8, 2025 19:20
@MikeNeilson MikeNeilson merged commit 77dbc01 into main Oct 8, 2025
9 of 10 checks passed
@MikeNeilson MikeNeilson deleted the patch_css_js branch October 8, 2025 19:44
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.

4 participants