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

Bump bootstrap from 4.6.2 to 5.0.0 #215

Merged
merged 17 commits into from
Nov 27, 2024

Conversation

dependabot[bot]
Copy link

@dependabot dependabot bot commented on behalf of github Oct 2, 2024

Resolve #236 #235 #227

Bumps bootstrap from 4.6.2 to 5.0.0.

Release notes

Sourced from bootstrap's releases.

v5.0.0

Highlights

#32155: Updated make-col() mixin to generate equal columns when no size is specified #32763: Added new color-scheme() mixin #33389: Dropdown menus now have option become clickable #33453: Added new docs footer #33548: Offcanvas header components are now vertically aligned #33549: Added offcanvas-top modifier #33634: Added support for .dropdown-items wrapped in <li>s #33626: Fix v5 regressions in tab dropdown functionality

🚀 Features

  • #32763: Add color-scheme mixin
  • #33389: Dropdown — Add option to make the dropdown menu clickable
  • #33549: Add offcanvas-top modifier

🎨 CSS

  • #32155: Add equal column mixin
  • #32763: Add color-scheme mixin
  • #33292: Make accordion icon rotation more natural
  • #33411: Fix validation feedback icon in select multiple
  • #33478: Make .nav-link color consistent when using buttons
  • #33482: Dropdown — Apply positioning only when Popper is not used
  • #33548: Vertically align offcanvas header components
  • #33549: Add offcanvas-top modifier
  • #33550: Spinner alignment changes
  • #33598: Hide validation icons from multiple selects
  • #33600: Have $form-check-input-border's default derive from $black
  • #33607: Reduce color-scheme complexity
  • #33642: use :read-only css selector instead [readonly] for consistency
  • #33658: fix: use list-group variable instead of alert
  • #33736: accordion: fix border-top on Firefox

☕️ JavaScript

  • #32439: Decouple BackDrop from modal
  • #33245: Decouple Modal's scrollbar functionality
  • #33249: Simplify Modal Config
  • #33250: Simplify ScrollSpy config
  • #33310: fix: make EventHandler better handle mouseenter/mouseleave events
  • #33389: Dropdown — Add option to make the dropdown menu clickable
  • #33429: Remove element event listeners through base component
  • #33451: Add missing things in hide method of dropdown
  • #33456: Use our isDisabled util on dropdown
  • #33466: Refactor dropdown's hide functionality
  • #33479: Fix dropdown escape propagation
  • #33496: Use cached noop function

... (truncated)

Commits

Dependabot compatibility score

You can trigger a rebase of this PR by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
    You can disable automated security fix PRs for this repo from the Security Alerts page.

Note
Automatic rebases have been disabled on this pull request as it has been open for over 30 days.

@dependabot dependabot bot added the dependencies Pull requests that update a dependency file label Oct 2, 2024
@dependabot dependabot bot force-pushed the dependabot/npm_and_yarn/bootstrap-5.0.0 branch 2 times, most recently from 37b7f8f to e3e95d0 Compare October 3, 2024 11:53
@blcham blcham force-pushed the dependabot/npm_and_yarn/bootstrap-5.0.0 branch from e3e95d0 to e66982c Compare October 3, 2024 14:37
@palagdan palagdan force-pushed the dependabot/npm_and_yarn/bootstrap-5.0.0 branch from ca7db4f to ede0a90 Compare October 6, 2024 11:50
@palagdan
Copy link
Collaborator

palagdan commented Oct 7, 2024

@blcham

I noticed that react-bootstrap isn't compatible with the new version of Bootstrap, so the first step was updating it to the latest version. This fixed issues with the icons and possibly other things, though I primarily noticed the icons.

Next, the jumbotron (the gray dashboard) was deprecated. Bootstrap suggests using their updated classes to achieve a similar design, but I remembered there's a nice component for this in the MUI library called Paper, which is already being used in other KBSS projects:

paper_example

I think it looks even better than before.

Another issue was the lack of bottom margin in input fields after the Bootstrap update. To fix this, I added a default mb-3 class to the input fields. We should apply the same fix to the s-forms.

I also replaced the deprecated InputGroup.Append with InputGroup.Text.

I replaced FormControl with FormSelect because FormControl started to look strange for select input after the update. . Also I replaced renamed bootstrap classes: "font-weight-bold text-lg-right align-self-center" with "fw-bold text-lg-end align-self-center".

The last issue I'm still working on is the big padding between buttons in a row, which only appears on larger screens:

padding_button

@blcham
Copy link

blcham commented Oct 7, 2024

@LaChope please have a look :)

@blcham
Copy link

blcham commented Oct 7, 2024

@palagdan

MUI library called Paper

I understand MUI as an alternative to Bootstrap, having different philosophies on how styling works and is applied in source code.
I would be afraid to combine them without having a really good reason to do that.

Another issue was the lack of bottom margin in input fields after the Bootstrap update. To fix this, I added a default mb-3 class to the input fields. We should apply the same fix to the s-forms.

Ok, you can work on that.

package.json Outdated Show resolved Hide resolved
src/components/dashboard/Dashboard.jsx Outdated Show resolved Hide resolved
@palagdan palagdan force-pushed the dependabot/npm_and_yarn/bootstrap-5.0.0 branch 2 times, most recently from 43fa980 to 2324fc8 Compare October 8, 2024 10:23
@palagdan
Copy link
Collaborator

palagdan commented Oct 8, 2024

normal_rm
Now it looks normal.

@palagdan palagdan requested a review from blcham October 8, 2024 10:33
Copy link

@blcham blcham left a comment

Choose a reason for hiding this comment

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

LGTM, but not sure if we can safely merge:

  • to me it seems safer to merge SForms migration to bootstrap 5 first, but if you are sure it will work correctly you can merge now

@blcham blcham requested review from LaChope and removed request for LaChope October 8, 2024 11:09
@palagdan palagdan force-pushed the dependabot/npm_and_yarn/bootstrap-5.0.0 branch from fccbc3b to 8c96ea1 Compare October 28, 2024 13:18
Copy link

@blcham blcham left a comment

Choose a reason for hiding this comment

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

LGTM, can be rebased+merged If my comments are resolved, just do not forget to use new version of SForms ... we need to bump it as well

<OverlayTrigger placement="right" overlay={tooltip}>
{icon()}
</OverlayTrigger>
<>
Copy link

Choose a reason for hiding this comment

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

The new source code looks kinda complicated... Why we had to do this? OverlayTrigger does not exist anymore?

Copy link

Choose a reason for hiding this comment

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

Or Bootstrap 5 does not support tooltips anymore?

Copy link

@blcham blcham Nov 1, 2024

Choose a reason for hiding this comment

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

By the way I see:
import { OverlayTrigger, Popover, Table } from "react-bootstrap";

in file

import { OverlayTrigger, Popover, Table } from "react-bootstrap";

How is it related? Isn't Popover alternative to tooltips? If so, we should definitely unify the way -- i.e., having Popover in both places or not having Popover at all !?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was struggling with OverlayTrigger. After migrating, the tooltip was rendering outside of the window, and I couldn't fix its position. In the latest commit, I replaced OverlayTrigger with the Overlay component from Bootstrap, which resolved the issue. I also removed the popover.

package.json Outdated Show resolved Hide resolved
Copy link

@blcham blcham left a comment

Choose a reason for hiding this comment

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

See my comments.

<OverlayTrigger placement="right" overlay={tooltip}>
{icon()}
</OverlayTrigger>
<>
Copy link

@blcham blcham Nov 1, 2024

Choose a reason for hiding this comment

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

By the way I see:
import { OverlayTrigger, Popover, Table } from "react-bootstrap";

in file

import { OverlayTrigger, Popover, Table } from "react-bootstrap";

How is it related? Isn't Popover alternative to tooltips? If so, we should definitely unify the way -- i.e., having Popover in both places or not having Popover at all !?

@blcham
Copy link

blcham commented Nov 1, 2024

@palagdan Just so i am able to add A/C i created #236 and referenced it here

@palagdan palagdan force-pushed the dependabot/npm_and_yarn/bootstrap-5.0.0 branch 2 times, most recently from ef78a57 to f0fe3b3 Compare November 4, 2024 12:07
@palagdan
Copy link
Collaborator

palagdan commented Nov 4, 2024

@blcham, I also updated Node to version 18 in the Dockerfile because there were some errors during the build process.

@palagdan palagdan force-pushed the dependabot/npm_and_yarn/bootstrap-5.0.0 branch from f0fe3b3 to 741f8fb Compare November 19, 2024 11:16
@blcham
Copy link

blcham commented Nov 19, 2024

👀

@palagdan
Copy link
Collaborator

@blcham

We need to publish a new version of SForms to NPM so I can test it with RM.

@blcham
Copy link

blcham commented Nov 19, 2024

@palagdan you can use:

  • concrete beta version if PR was merged to the main
  • concrete alpha version if PR is still open

@blcham
Copy link

blcham commented Nov 19, 2024

Not sure, but believe you can deduce "name of version" from hash of the last commit

@palagdan palagdan requested a review from blcham November 25, 2024 18:54
@palagdan
Copy link
Collaborator

@blcham
I replaced an Overlay with OverlayTrigger, so I think it is ready to be merged.

@blcham blcham force-pushed the dependabot/npm_and_yarn/bootstrap-5.0.0 branch from e4514ef to a447c6c Compare November 26, 2024 15:14
@blcham blcham self-requested a review November 26, 2024 22:03
Copy link

@blcham blcham left a comment

Choose a reason for hiding this comment

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

@palagdan LGTM, but I do not understand why you did not upgrade s-forms in package.json to the version where SForms is migrated to bootstrap 5.0.0

You can use s-forms 0.8.0 for it (I have just released this version)

@palagdan
Copy link
Collaborator

palagdan commented Nov 27, 2024

@blcham

I thought we would discuss first which version of SForms to use. I checked https://www.npmjs.com/package/@kbss-cvut/s-forms and there is no 0.8.0 there.

@blcham
Copy link

blcham commented Nov 27, 2024

@palagdan sry, it should be there now -- https://www.npmjs.com/package/@kbss-cvut/s-forms/v/0.8.0

@palagdan palagdan force-pushed the dependabot/npm_and_yarn/bootstrap-5.0.0 branch from a447c6c to 186a8f9 Compare November 27, 2024 11:12
@palagdan palagdan requested a review from blcham November 27, 2024 11:21
@blcham blcham merged commit 237d369 into main Nov 27, 2024
2 checks passed
@blcham blcham deleted the dependabot/npm_and_yarn/bootstrap-5.0.0 branch November 27, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump bootstrap to 5.0.0
3 participants