Skip to content

Migrate modals from bootstrap-vue to vue-final-modal - #1849

Merged
ang-zeyu merged 39 commits into
MarkBind:masterfrom
jovyntls:vue-final-modal
Mar 27, 2022
Merged

Migrate modals from bootstrap-vue to vue-final-modal#1849
ang-zeyu merged 39 commits into
MarkBind:masterfrom
jovyntls:vue-final-modal

Conversation

@jovyntls

@jovyntlsjovyntls commented Mar 23, 2022

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Part of #1702

Overview of changes:
Updates modals to use the vue-final-modal package for modals instead of bootstrap-vue.

  • the modal transition CSS classes are moved to from markbind.css to Modal.vue with more specific selectors
  • bootstrapVueProcessor.js is fully removed
    • the _renameSlot method is moved to renameSlot in vueSlotSyntaxProcessor.js
  • Vue was upgraded from v2.6.11 to v2.6.14 to meet the dependencies of the latest version of vue-final-modal (v2.4.1)

DIscussion on using vue-final-modal and removing bootstrap-vue can be found on #1702.

Anything you'd like to highlight / discuss:
Deprecation of modal-footer and modal-header slots:
The docs state that these slot are deprecated their implementations had not been removed.
It seems like deprecating these slots may be an unrelated change, hence these slots are re-implemented in Modals and can be removed in a future PR for fully deprecating modal-header and modal-footer.

Testing instructions:
The existing documented attributes for modals should work as described.

Proposed commit message: (wrap lines at 72 characters)

Use vue-final-modal for modal components
Modals are implemented using bootstrap-vue directives.
bootstrap-vue has dependencies on specific versions of Bootstrap and
Vue, making it hard for MarkBind to migrate to Bootstrap 5, or Vue 3.
vue-final-modal has minimal dependencies, and supports both Vue 2 and 3.
Let's use vue-final-modal for modals instead. Doing so allows for future
migrations to Bootstrap 5 or Vue 3, without having to wait for
bootstrap-vue to migrate first. Vue is also upgraded from v2.6.11 to v2.6.14, to meet the dependencies
of vue-final-modal v2.4.1.

Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jovyntlsjovyntls changed the title Migrate modals from vue-bootstrap to vue-final-modalnMigrate modals from vue-bootstrap to vue-final-modalMar 23, 2022
@jovyntls
jovyntls marked this pull request as ready for review March 23, 2022 08:56
Comment threadpackages/vue-components/package.json Outdated
@jovyntls

Copy link
Copy Markdown
ContributorAuthor

Thanks for pointing it out @jonahtanjz , didn't realise the snapshot tests were a thing! I've changed the styles to global styles and added snapshot tests :)

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @jovyntls! The tests look comprehensive :)

LGTM 👍

Will close and reopen this PR to restart the CI, looks like some issues with github recently 😅

@jonahtanjzjonahtanjz reopened this Mar 25, 2022
@jonahtanjzjonahtanjz added this to the 4.0 milestone Mar 25, 2022
@ang-zeyu
ang-zeyu self-requested a review March 25, 2022 04:11
@jovyntlsjovyntls changed the title Migrate modals from vue-bootstrap to vue-final-modalMigrate modals from bootstrap-vue to vue-final-modalMar 25, 2022

@ang-zeyuang-zeyu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work, just 2 nits:


The number of package-lock.json changes introduced in merging conflicts c3e678f seems odd (compared with the earlier 2 commits changing package-lock.json)

<vue-final-modal
v-if="isMounted"
v-model="show"
ssr

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to check, was ssr=false tried?

Presumably it prevents the modal from getting rendered during the ssr cycles, but I have been getting errors with it (might be a bug over there). The isMounted way seems like a good alternative.

Should we set ssr=false for clarity? (shouldn't be rendered during ssr) (maybe with a comment as well)

https://github.com/vue-final/vue-final-modal/blob/master/lib/VueFinalModal.vue#L3

@jovyntlsjovyntlsMar 26, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yup! When isMounted is used, both ssr and ssr=false work. (Without isMounted, both don't work.) I've added a comment to clarify.

Just to check my understanding, why should modals not be rendered during SSR cycles? I used browser devtools to check for SSR hydration issues, not sure if there's any other way to detect SSR issues.

EDIT: ssr=false breaks the snapshot tests, so I reverted the change

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

EDIT: ssr=false breaks the snapshot tests, so I reverted the change

Ok, ugh, the issue seems to be that $vfm (being a singleton) isn't really testable. Wonder if we should transfer this once we have e2e tests setup in the other PR. @jonahtanjz 🤔

Ok with snapshot tests now though. Let's update the comment as appropriate too.

Just to check my understanding, why should modals not be rendered during SSR cycles? I used browser devtools to check for SSR hydration issues, not sure if there's any other way to detect SSR issues.

That depends, in MarkBind's case we've decided to discard modals from indexing, per the updates you did in algolia.js.

For this implementation https://github.com/vue-final/vue-final-modal/blob/master/lib/VueFinalModal.vue#L3, the ssr option seems to render a display: none modal. This might be useful if you want to include modal content inside search engine results.

Another case is if you want to show the modal on-page-load (rarely, since popups are triggered by user action usually), you would need to render it during ssr as well. (by setting the initial value of show=true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

per the updates you did in algolia.js.

EDIT on this: sorry, just realised with this discussion these selectors have to be retargeted / no longer necessary for SSR, something we missed out in #1534. (same rationale of excluding from indexing still applies though)

I've added it on to #1815.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I see, thank you so much for the explanation!
Have updated the comment too :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wonder if we should transfer this once we have e2e tests setup in the other PR. @jonahtanjz 🤔

I think we can keep the current snapshot tests for now since the e2e tests are focused on the live preview part (this may not be relevant there?). We can consider moving this once we have more e2e tests for the other frontend components as well.

@jovyntls

Copy link
Copy Markdown
ContributorAuthor

I tried checking out package-lock.json to the latest commit on master and then npm run setup, but it still gives 48k insertions and 25k deletions. O.o Not sure why there is such a large change since the only changes to the package.json files are adding vue-final-modal and upgrading the patch version of vue (along with vue-server-renderer, vue-template-compiler).

Would this be an issue for the version history of package-lock.json?

@jovyntlsjovyntls mentioned this pull request Mar 26, 2022
9 tasks
@ang-zeyu

Copy link
Copy Markdown
Contributor

I tried checking out package-lock.json to the latest commit on master and then npm run setup, but it still gives 48k insertions and 25k deletions. O.o Not sure why there is such a large change since the only changes to the package.json files are adding vue-final-modal and upgrading the patch version of vue (along with vue-server-renderer, vue-template-compiler).

Would this be an issue for the version history of package-lock.json?

Hmm, just tried a fresh (wiped all node_modules -- could try this) install, I'm not getting any updates on the main branch.

@jovyntls

Copy link
Copy Markdown
ContributorAuthor

Hmm, just tried a fresh (wiped all node_modules -- could try this) install, I'm not getting any updates on the main branch.

I tried:

  • restore package-lock.json to the version on the master branch
  • delete all node_modules, including those in packages/*
  • npm run setup

Still getting the excessive changes, could be that I've messed up my config somehow, though I recently uninstalled and reinstalled the newest version of npm.

@ang-zeyu

Copy link
Copy Markdown
Contributor

👀 Just did a fresh clone + install on another os (ubuntu) but still not getting any changes.

Just in case, try npm -V. Might be an issue with multiple node/npm installations and your PATH configuration.

If it still persists think we could get @jonahtanjz to verify as well

@jonahtanjz

Copy link
Copy Markdown
Contributor

If it still persists think we could get @jonahtanjz to verify as well

I'm able to get the correct changes in package-lock.json by restoring from the master branch and running npm run setup. I've put up the one I got on github here. Maybe you can try replacing it with this and run npm run setup again to see if that helps?

@jovyntls

Copy link
Copy Markdown
ContributorAuthor

Ah ok, a fresh clone and re-installation worked for me! Not sure what the problem with the previous clone was.

Thank you @ang-zeyu and @jonahtanjz for the help :)

@ang-zeyu
ang-zeyu merged commit a68a86d into MarkBind:masterMar 27, 2022
@jovyntlsjovyntls mentioned this pull request Apr 4, 2022
9 tasks
@jovyntlsjovyntls mentioned this pull request Apr 15, 2022
51 tasks
Sign up for freeto 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.

3 participants

@jovyntls@ang-zeyu@jonahtanjz