Skip to content

Conversation

@jovyntls
Copy link
Contributor

@jovyntls jovyntls commented Mar 27, 2022

What is the purpose of this pull request?

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

Part of #1702 , by removing the dependency on bootstrap-vue

Overview of changes:

  • Popovers use the Menu component from Floating Vue
  • Tooltips use the Tooltip component from Floating Vue
  • Default value of placement has been changed to placement=top to preserve the same behaviour. floating-vue has the option for placement=auto, but this tends to render popups to the right in small containers (where they previously would be on top). Since floating-vue automatically repositions popovers even for non-auto placements, placement=top is the closest to the original behaviour.

Anything you'd like to highlight / discuss:
Should the full removal of bootstrap-vue be done in a separate PR?

Testing instructions:
There should be no noticeable change in behaviour or UI for popovers and tooltips

Proposed commit message: (wrap lines at 72 characters)

Migrate popovers and tooltips to floating-vue

Popovers and tooltips are implemented using bootstrap-vue. 

bootstrap-vue has dependencies on both Vue and Bootstrap, and has not
migrated the latest versions. This is a blocker for MarkBind's
migration.

Let's use floating-vue's Menu and Tooltip components for Popovers and
Tooltips respectively. floating-vue supports both Vue 2 and Vue 3, and
has fewer dependencies. This allows us to migrate to Vue 3 and 
Bootstrap 5 more easily in the future. 

Checklist: ☑️

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

@jovyntls jovyntls changed the title Floating UI popups Migrate popovers and tooltips to floating-vue Mar 27, 2022
@jovyntls jovyntls marked this pull request as ready for review March 27, 2022 08:26
@ryoarmanda ryoarmanda requested review from kaixin-hc and ong6 March 28, 2022 06:33
Copy link
Contributor

@ong6 ong6 left a comment

Choose a reason for hiding this comment

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

LGTM! Just a few questions:

  1. Is bootstrap-vue still in use? There are still some remnants of it here.
  2. If its not in use maybe there is a need to run npm run updatetest also

@jovyntls
Copy link
Contributor Author

  1. Is bootstrap-vue still in use? There are still some remnants of it here.

Yep! Mentioned in the PR description that it can be removed in a separate PR to keep this one small, what do you think? I'm not sure if it's best to just remove it along with this PR too since it's related.

@ong6
Copy link
Contributor

ong6 commented Mar 28, 2022

  1. Is bootstrap-vue still in use? There are still some remnants of it here.

Yep! Mentioned in the PR description that it can be removed in a separate PR to keep this one small, what do you think? I'm not sure if it's best to just remove it along with this PR too since it's related.

Ah missed that part in your description XD. I think it makes more sense to remove it in a separate PR, since quite a few tests files will be affected by it too.

@kaixin-hc
Copy link
Contributor

There are slight functionality differences, though my opinion is that they are negligible and we can proceed. Thanks for your hard work Jovyn!

  1. On small screen sizes/mobile, the tooltip/popover as seen in the Netlify preview is not moved, leading to the text being cut off; on the current master user guide, it is moved.
    deploy preview shows it stretching to the left MarkBind shows it being repositioned to move inwards

This surprised me, especially as I saw in your issue description that floating-vue should auto-reposition?

  1. Tooltips are noticeably transparent in the new implementation rather than opaque (I think this is a positive change)

Screenshot 2022-03-28 at 11 49 22 PM

On a side note, I noticed that in the deploy preview all the modals on the same page in the docs disappeared 🤔 I wonder why that is?

@jovyntls
Copy link
Contributor Author

jovyntls commented Mar 28, 2022

@kaixin-hc Thanks for the catch! I moved the default <slot> in Triggers.vue to match the API of VTooltip and VPopover, but forgot that the <slot> was still needed by Modals.

Didn't realise the decrease opacity of tooltips had changed too D: Will maintain the same styles by replicating the previous opacity.

For the positioning issue, the auto-repositioning is working - the issue seems to be due to different ways of handling cases where the screen is small.

Currently on the master branch, when the tooltip "flips" and there's still not enough space, the tooltip itself will move in (see the red circle no longer on the edge of the button):
Screenshot 2022-03-29 at 12 37 28 AM

When floating-vue tries to reposition by flipping the tooltip, there's not enough space on the right either so it "gives up" and stays on the left. However, the tooltip stays on the edge of the button (see red circle), resulting in a cut-off tooltip:

Screenshot 2022-03-29 at 12 38 02 AM

As a fix, the shift-cross-axis property in floating-vue seems to fix this. Using shift-cross-axis will move the tooltip instead of flipping. This will ensure that the contents are always readable, but it may block the trigger text (same scenario as the first image). However, I think this may be a worthwhile tradeoff?

@jovyntls jovyntls requested a review from ong6 March 30, 2022 16:09
Copy link
Contributor

@ong6 ong6 left a comment

Choose a reason for hiding this comment

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

Nice work overall, no issues on the code side but I found that the user side documentation has yet to be updated to remove the bootstrap reference.

For example here, the modal sizes here still link to bootstrap, perhaps we can update it?

@jovyntls
Copy link
Contributor Author

Nice work overall, no issues on the code side but I found that the user side documentation has yet to be updated to remove the bootstrap reference.

For example here, the modal sizes here still link to bootstrap, perhaps we can update it?

Oops missed that out 😅 I can update it in a separate PR (maybe the one to fully remove bootstrap-vue since that will involve updating docs as well)!

@kaixin-hc
Copy link
Contributor

As a fix, the shift-cross-axis property in floating-vue seems to fix this. Using shift-cross-axis will move the tooltip instead of flipping. This will ensure that the contents are always readable, but it may block the trigger text (same scenario as the first image).

I agree that this is a worthwhile tradeoff!

WRT the documentation, I think it's better to have it reflect the actual functionality in this PR? Or I suppose we can make an issue for it or add it to an existing issue to make sure it doesn't get forgotten

@jovyntls
Copy link
Contributor Author

WRT the documentation, I think it's better to have it reflect the actual functionality in this PR? Or I suppose we can make an issue for it or add it to an existing issue to make sure it doesn't get forgotten

Which documentation are you referring to? 😅

@kaixin-hc
Copy link
Contributor

WRT the documentation, I think it's better to have it reflect the actual functionality in this PR? Or I suppose we can make an issue for it or add it to an existing issue to make sure it doesn't get forgotten

Which documentation are you referring to? 😅

ah the thing that @ong6 raised, the table with modal sizes for example?

@jovyntls
Copy link
Contributor Author

jovyntls commented Mar 31, 2022

WRT the documentation, I think it's better to have it reflect the actual functionality in this PR? Or I suppose we can make an issue for it or add it to an existing issue to make sure it doesn't get forgotten

Which documentation are you referring to? 😅

ah the thing that @ong6 raised, the table with modal sizes for example?

Yep! I can address it in a separate PR (the one removing bootstrap-vue entirely) since it's not directly related to popovers and tooltips. For the modals, to the user it should work and look the same, just that the implementation is no longer using bootstrap modals.

@ong6 ong6 merged commit efa4f04 into MarkBind:master Apr 1, 2022
@ryoarmanda ryoarmanda added this to the 4.0 milestone Apr 5, 2022
@jovyntls jovyntls mentioned this pull request Apr 15, 2022
51 tasks
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.

4 participants