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 #1702

Closed
damithc opened this issue Jan 8, 2022 · 16 comments · Fixed by #1882
Closed

Bootstrap 5 #1702

damithc opened this issue Jan 8, 2022 · 16 comments · Fixed by #1882

Comments

@damithc
Copy link
Contributor

damithc commented Jan 8, 2022

Looks like Bootstrap has moved to v5

Some new things I noticed in passing: built-in support for icons, themes

@damithc damithc added the s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding label Jan 8, 2022
@ang-zeyu ang-zeyu added this to Slightly Higher Priority in Big Picture Jan 14, 2022
@ang-zeyu ang-zeyu added d.moderate p.Medium and removed s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding labels Jan 14, 2022
@ang-zeyu
Copy link
Contributor

Similar to #1716. Can be worked on incrementally.

@jovyntls jovyntls self-assigned this Mar 16, 2022
@jovyntls
Copy link
Contributor

Hello, I'm interested in working on this :)

I notice that Bootstrap 5 has quite a few breaking changes, which will break/change many of our styles. I think fixing the deprecated parts incrementally would be a good idea, otherwise the PR will be too big and hard to review. However, I realise that with one PR for each fix, MarkBind styles will be buggy/broken until the final PR is merged. I'm not sure what will be a good way to split up the migration?

@jonahtanjz
Copy link
Contributor

I notice that Bootstrap 5 has quite a few breaking changes, which will break/change many of our styles. I think fixing the deprecated parts incrementally would be a good idea, otherwise the PR will be too big and hard to review. However, I realise that with one PR for each fix, MarkBind styles will be buggy/broken until the final PR is merged. I'm not sure what will be a good way to split up the migration?

Generally I don't think it's a good idea to split this up into multiple PRs. Like you have mentioned, this would mean having some functionalities/styles that will not work until the final PR is merged, which can take some time and also not able to test whether the newly introduced components are working as expected.

One way to do it incrementally is to open a single PR, then push changes to it periodically (with properly organised commits). Maybe after migrating a few components, you can ping someone to review it. Once it is good, you can start working on other components and continue this process until all the deprecated parts have been migrated. This should make it easier for reviewers as the changes that they will be looking at are much smaller and also ensure that all the changes can be properly tested before merging with the master branch :)

@damithc
Copy link
Contributor Author

damithc commented Mar 16, 2022

A long-lived feature branch might be suitable for this. That is, create a separate branch (in the main repo) for this task and keep sending PRs against that branch (instead of the master branch). Once everything has been migrated, the feature branch can be merged back to the master branch.

@jovyntls
Copy link
Contributor

@jonahtanjz @damithc Thank you for the insights! A long-lived feature branch sounds like a good idea - I'll create a bootstrap-v5-migration branch in the main repo.

@jonahtanjz
Copy link
Contributor

@jovyntls @damithc @ang-zeyu

While looking through the PR #1834, I noticed that our vue components are using bootstrap-vue for styling. From their current website, it seems that bootstrap-vue currently does not support Bootstrap 5 as of now. We may have to put this issue on hold until bootstrap-vue supports v5 or move to an alternative library that supports v5.

This is their current issue tracker bootstrap-vue/bootstrap-vue#5507

@damithc
Copy link
Contributor Author

damithc commented Mar 17, 2022

This is their current issue tracker bootstrap-vue/bootstrap-vue#5507

As mentioned in that thread, we have to consider the possibility that bootstrap-vue might die off and consider alternatives :-(

@jonahtanjz
Copy link
Contributor

jonahtanjz commented Mar 17, 2022

Some alternatives that we can consider:

  1. BootstrapVue3

    • Still quite early, in alpha version (may not be stable yet)
    • Most similar to bootstrap-vue (don't have to change many components)
    • Only supports Vue 3 and Bootstrap 5 (we have to migrate both Vue and Bootstrap at the same time for this to work)
  2. CoreUI Bootstrap Vue

    • Looks quite new as well, currently in beta version
    • Will have to change certain components (syntax, naming etc)
    • Looks like will have to use Vue 3 and Bootstrap 5 as well

These are the 2 alternatives I've found. Both require Vue 3 and Bootstrap 5 so we have to upgrade them before we can use the libraries.

Feel free to suggest other alternatives if there are any better options.

@jovyntls
Copy link
Contributor

It looks like the bootstrap-vue components are Popovers, Tooltips and Modals, all of which are supported by Bootstrap. Is it viable to remove the dependency on a Vue UI library and instead write these components using Bootstrap only?

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Mar 17, 2022

It looks like the bootstrap-vue components are Popovers, Tooltips and Modals, all of which are supported by Bootstrap. Is it viable to remove the dependency on a Vue UI library and instead write these components using Bootstrap only?

No objections, moving our components away from bootstrap-vue should also facilitate #903 in the future, if that's what we want to proceed with (I'll drop a separate summary post there shortly).

Came across this https://floating-vue.starpad.dev/guide/component.html#dropdown (based on floating-ui), which dosen't seem to rely on bootstrap, and has vue v2 support as well. Though there's only popovers / tooltips in there, and no header option. There's likely also a viable one for modals around somewhere =P

@jovyntls
Copy link
Contributor

For modals, we can look into:

  • Vue Final Modal
    • both Vue 2 and Vue 3 supported; minimal dependencies
    • a lot of functionality (e.g. draggable and resizable modals), though we may not need it
  • Creating a Vue component for it based on the Bootstrap modal

Other alternatives explored but unviable:

@ang-zeyu
Copy link
Contributor

For modals, we can look into:

  • Vue Final Modal

    • both Vue 2 and Vue 3 supported; minimal dependencies
    • a lot of functionality (e.g. draggable and resizable modals), though we may not need it

Try including the minimal functionalities and see how the bundle size increases. If not too big might be ok.

We considered this in the past but decided against it as the bootstrap-vue code is rather complex to maintain (written in terms of lower level render apis).

Another option is to bring back the minimal vue-strap modal implementations we had, though I think it would be quite a bit of work to bring this up to standard against more focused external libraries (e.g. accessibility labels, ...).

@jovyntls
Copy link
Contributor

jovyntls commented Mar 18, 2022

Try including the minimal functionalities and see how the bundle size increases. If not too big might be ok.

I tried adding vue-final-modal and creating a minimal component using their quickstart example, then running npm run build:web (is this is the right way to check bundle size?)

  • Size of markbind.min.js before: 191KB
  • Size of markbind.min.js after: 217K
    • Results in the following warning:
      image
  • (No change in markbind.min.css size)

I'm not sure if this is considered a large increase?

@ang-zeyu
Copy link
Contributor

I think this looks fine 🎉, was expecting / worried about a 50-100kb increase.

@jovyntls
Copy link
Contributor

Alright! In that case, would this be a viable plan for porting components away from vue-bootstrap:

The Bootstrap v5 migration would be put on hold until these are done?

@ang-zeyu
Copy link
Contributor

sounds good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Big Picture
Completed
Development

Successfully merging a pull request may close this issue.

4 participants