Skip to content

Upgrade webui to Bootstrap 5 #360

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 27, 2021
Merged

Upgrade webui to Bootstrap 5 #360

merged 4 commits into from
May 27, 2021

Conversation

@saurori
Copy link
Collaborator

@saurori saurori commented May 27, 2021 •
edited
Loading

Description

This PR updates the webui from Bootstrap 3 to Bootstrap 5. The CSS and HTML was updated, but not the JS (only the Dropdown plugin is used).

Related Issue: #359

Notes

  • I tried to fix and update all RTL CSS but there's a chance I missed things. I didn't touch webui/static/application-rtl.css at all.
  • Fixed a bug when TextDirection was missing in locales yml it would set the <html dir="TextDirection">.
  • I removed some HTML from the navbar related to navbar-livereload as it did not seem to be in use.
  • I modified the Bootstrap 3 JS data- selector names so it lines up with CSS selectors in Bootstrap 5. Bootstrap 5 does not use jQuery but the Faktory UI still relies on it so it was easier to leave that in place vs including additional JS to support the new dropdown JS code.

@mperham
Copy link
Collaborator

mperham commented May 27, 2021 •
edited
Loading

The Scheduled page lists jobs which contain the at attribute and have not yet executed.

https://github.com/contribsys/faktory/wiki/The-Job-Payload#options

With FWR, you'd do something like this: SomeJob.perform_in(1.hour, "some args")

@saurori
Copy link
Collaborator Author

saurori commented May 27, 2021

Ah ok. Tested that, the page looks fine.

mperham
mperham reviewed May 27, 2021
View reviewed changes
@mperham
Copy link
Collaborator

mperham commented May 27, 2021

Lots of nice work here, thanks.

I noticed you didn't touch application-rtl.css. Did you test RTL? I believe you can do so by selecting an RTL language on the /debug page if you didn't already.

I will walk through the pages later today to double check the changes.

@saurori
Copy link
Collaborator Author

saurori commented May 27, 2021

I tested RTL a bunch, as much as I know how (native English speaker). It all looks good to me but there may be some subtleties I missed.

@saurori
Copy link
Collaborator Author

saurori commented May 27, 2021

I did another pass over application-rtl.css and mostly removed dead CSS but also fixed a few small layout bugs / replaced items with native Bootstrap RTL classes. dcca7ef

@mperham
Copy link
Collaborator

mperham commented May 27, 2021

There are a few minor issues here and there. Some before and after images:

Before
before

After
after

Button color is gone. The table rows are colored differently, I don't mind that but do they need a little more contrast vs the page body color?

@mperham
Copy link
Collaborator

mperham commented May 27, 2021

But overall, where things look different, they look better to my eye so I'm quite pleased!

I think the Dashboard interval slider needs to retain its ltr class. That class has been there for years and no one has complained so please restore it. I'm happy to remove it if someone with rtl experience tells me it looks wrong.

@saurori
Copy link
Collaborator Author

saurori commented May 27, 2021

  1. Buttons: BS5 got rid of button-default, will fix that
  2. For whatever reason, BS5 switches the table row colors of :odd vs :even. Otherwise it's close to the same as before. I agree the grey color on tables could use more contrast. table-light looks a bit better, see variants or a custom color...

@mperham
Copy link
Collaborator

mperham commented May 27, 2021

Thanks, I'm ready to merge this. Are you happy with it or do you need more time?

I think you've earned commit bit with this work. You are welcome to make changes, just please open a PR for anything you do change so I can review and we have a record of why things changed.

@saurori
Copy link
Collaborator Author

saurori commented May 27, 2021

👍 I think it's ready to merge.

@mperham mperham merged commit 05230d2 into contribsys:master May 27, 2021
@mperham
Copy link
Collaborator

mperham commented May 27, 2021

Oh, and any significant change should have a line in Changes.md. Can you add one for this PR?

@saurori
Copy link
Collaborator Author

saurori commented May 27, 2021

Changelog: #363

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mperham mperham mperham left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@saurori @mperham