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

Replace bootstrap related local libraries with npm dependencies #22

Merged
merged 3 commits into from
Nov 18, 2023

Conversation

krausvo1
Copy link
Contributor

I have replaced bootstrap related local libraries with npm dependencies, namely bootstrap, bootstrap-dialog and selectize.

I have kept the same versions of bootstrap and bootstrap-dialog, but I have upgraded selectize from 0.12.1 to 0.14.0., because:

  • selectize 0.12.1 (at least its npm version) has dependencies microplugin and sifter that need to be provided by us (they already are, but not explicitly in commons package.json) and their paths need to be explicitly mentioned in downstream project's require configuration (forgerock-ui-mock/src/main.js).
  • These dependencies are bundled together with @selectize/selectize since 0.14.0 (package was renamed since 0.13.x), so we don't need to care about them. This is also the only major breaking change since 0.12.1, so nothing should break with this upgrade (see changelog).
  • I have also considered upgrading to the latest version, but there have been few stale issues introduced in 0.15.x without any reaction from the maintainer, so I would rather stay at 0.14.0.

I have quickly tested the mock project and everything seems to work fine as it did before.

Copy link
Member

@pavelhoral pavelhoral left a comment

Choose a reason for hiding this comment

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

LGTM

There might be something wrong with Selectize styles in a sense that focus does not have a proper CSS, However this was broken before this PR. I would love to check this before merging.

Also another thing I would like to check if we need the full Bootstrap JS. But I guess that we do.

@krausvo1
Copy link
Contributor Author

Also another thing I would like to check if we need the full Bootstrap JS. But I guess that we do.

I don't know if we need it, just for the record - the JS components mentioned in config.json that was used to generate the former Bootstrap JS bundle contains all the JS components that Bootstrap 3 offers. So I did not bother importing only specific components.

Also, I suppose we can now delete that config file since we won't need it anymore...?

@pavelhoral
Copy link
Member

There might be something wrong with Selectize styles in a sense that focus does not have a proper CSS,

It is behaving correctly - component looks the same as on Selectize's demo page.

@pavelhoral pavelhoral merged commit 83f2326 into WrenSecurity:main Nov 18, 2023
2 checks passed
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.

2 participants