-
Notifications
You must be signed in to change notification settings - Fork 123
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
Migrate to bootstrap-vue popovers #1033
Conversation
https://deploy-preview-1033--markbind-master.netlify.com/userguide/usingcomponents#popovers All popovers trigger only on click for now for better testing (can open multiple at once, inspect element more easily). |
Since the typical use case for a popover is to include really small tidbits of info, I don't think the cost for this is significant I like the idea of moving the processing work to /markbind, but why a plugin though? Could we try integrating this into The tricky part would be that That said, that's just my two cents, but at the very least this should be a stage in the Another possible issue is the way |
Thanks for the review!
This also wouldn't work. Consider a trigger, followed by the actual popover element. When the trigger component is parsed, the popover is not yet parsed. Later on, there's no way to "go back" and edit it back in. If we were to go that route then we would need two maps, one storing all the triggers, and another storing all the popovers, and each time we parse either we need to check if they have been referenced by another popover/trigger. Maybe I'll try this.
Yes, the main reason for it being a plugin is so that it has access to the entire DOM. Yeah and noted regarding the dependent files too, could probably have a counter counting the number of included files too. |
Hmm, there shouldn't be a need to 'go back'. Pseudo code:
|
Yeah I got that, but the problem is in Like
or
I believe it still can be a solution if we use two maps, a |
Hmm, isn't |
Ah yes, you're right. Thanks! |
I realised this doesn't work well also: The popover's content and title might contain other stuff that need to be processed if slots are used, like <popover>
<div slot="content><include src="file.md"/></div> Hover me </popover> but then the map wouldn't have access to the processed popover. I can add the not-yet-parsed popover into a map during the preprocess stage no problem, but that will not give access to the actual parsed popover. |
True. In that case, I can't think of a better implementation than this at the moment. An alternative would be to do this on the front end, by creating b-popovers as many times as there are triggers. But this is likely to be more heavy on the client, which we should try to avoid. |
Plugin Duplication |
The plugin works, but we'd run into issues along the way: the footnotes plugin uses popovers, so we'd want the plugins to render footnotes into popovers, and then transform all popovers into bootstrap-vue components. So we need to control the order in which plugins are executed. Is there a way to do this now? |
We could insert this a seperate stage before We could also use this for same-page id conflict resolution as well, e.g. a |
On another note though, was using It seems like bootstrap-vue popovers & tooltips aren't suited to our use case ( multiple triggers for one popover ). That said if the long term goal is to exploit the large amount of features bootstrap-vue has, and not just for popovers / tooltips, then I don't think there's an issue here. |
I'm not sure, does the current implementation respect load order? If not we could make a separate issue and PR to improve plugins, possibly also adding more hooks throughout the |
27529a9
to
716b593
Compare
Okay after some days I realised a better solution: v-b-popover directives allow us to pass in an object, so that we can provide functions that return strings for content and title: This allows us to have the best of both words: no more duplication of any popovers 🎉 and the subjectively better positioning offered by bootstrap-vue. One downside is that we won't have support for the View the popovers here: This solution is a lot simpler than my previous solution, and does not require the iffy generation of ids for popovers at all. |
I think it's an acceptable compromise. |
Ready for review! Some breaking changes: Contextmenu (right click) no longer a trigger bootstrap-vue modals don't support the width attribute Popovers now don't have the |
# Conflicts: # asset/css/markbind.css # test/functional/test_site/expected/markbind/css/markbind.css # test/functional/test_site/expected/testTooltipSpacing.html # test/functional/test_site_algolia_plugin/expected/markbind/css/markbind.css # test/functional/test_site_convert/expected/markbind/css/markbind.css # test/functional/test_site_expressive_layout/expected/markbind/css/markbind.css # test/functional/test_site_templates/test_default/expected/markbind/css/markbind.css # test/functional/test_site_templates/test_minimal/expected/markbind/css/markbind.css
Took a brief look at the code, looks good 👍. Functionality wise, noticed some sort of flickering when the placement is Is it possible to use an empty |
I've removed the cancel button (just added the Seems like bootstrap-vue has fixed the positioning thing! I updated bootstrap-vue.js and bootstrap-vue.css to the latest versions and they work better. They even have support for the hovering over of popovers, so that you can select text inside them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach seems fine, but I think it would be great if we can leave more context for people who might come across this in the future and wonder why we are doing all of these transformations. Just a couple of questions about variables!
Also, are the issues you mentioned in the PR regarding modals and tooltips resolved? I gave it a try on the netlify site and it seems to work.
asset/css/bootstrap-vue.min.css
Outdated
/*! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be preferable to update the bootstrap-vue assets in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! I'll revert the change.
*/ | ||
function _transformSlottedComponents(element) { | ||
element.children.forEach((c) => { | ||
const child = c; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be missing something here, but could we just rename the c
parameter to child
directly instead? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, It's because of our eslint rule no-param-reassign
. We do seem to be assigning properties a lot though, I'll raise an issue on this.
*/ | ||
|
||
function _parseTrigger(element) { | ||
const el = element; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for el
and element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in the above we seem to be favouring explicitness (child
instead of c
), but here we seem to be favouring terseness. Is there a reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the child function then, to keep it more consistent.
Thanks for the review! Yeah modals and tooltips all work now, updated the PR. I'll add in more documentation on the transformations do too |
I think this is a bug on master. Other netlify builds that have been rebased on top of master has the same issue. |
It's a new feature in #991, when the user hovers over too-long code it will automatically wrap |
This reverts commit 9697c8f
I should probably open an issue ( #1077) 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…nvert-to-code-block * 'master' of https://github.com/MarkBind/markbind: Allow changing parameter properties (MarkBind#1075) Custom timezone for built-in timestamp (MarkBind#1073) Fix reload inconsistency when updating frontmatter (MarkBind#1068) Implement an api to ignore content in certain tags (MarkBind#1047) Enable AppVeyor CI (MarkBind#1040) Add heading and line highlighting to code blocks (MarkBind#1034) Add dividers and fix bug in siteNav (MarkBind#1063) Fixed navbar no longer covers modals (MarkBind#1070) Add copy code-block plugin (MarkBind#1043) Render plugins on dynamic resources (MarkBind#1051) Documentation for Implement no-* attributes for <box> (MarkBind#1042) Migrate to bootstrap-vue popovers (MarkBind#1033) Refactor preprocess and url processing functions (MarkBind#1026) Add pageNav to Using Plugins Page (MarkBind#1062) # Conflicts: # docs/userGuide/syntax/siteNavigationMenus.mbdf
* 'master' of https://github.com/MarkBind/markbind: 2.12.0 Update outdated test files Update vue-strap version to v2.0.1-markbind.37 Fix refactor to processDynamicResources (MarkBind#1092) Implement lazy page building for markbind serve (MarkBind#1038) Add warnings for conflicting/deprecated component attribs (MarkBind#1057) Allow changing parameter properties (MarkBind#1075) Custom timezone for built-in timestamp (MarkBind#1073) Fix reload inconsistency when updating frontmatter (MarkBind#1068) Implement an api to ignore content in certain tags (MarkBind#1047) Enable AppVeyor CI (MarkBind#1040) Add heading and line highlighting to code blocks (MarkBind#1034) Add dividers and fix bug in siteNav (MarkBind#1063) Fixed navbar no longer covers modals (MarkBind#1070) Add copy code-block plugin (MarkBind#1043) Render plugins on dynamic resources (MarkBind#1051) Documentation for Implement no-* attributes for <box> (MarkBind#1042) Migrate to bootstrap-vue popovers (MarkBind#1033) Refactor preprocess and url processing functions (MarkBind#1026) Add pageNav to Using Plugins Page (MarkBind#1062)
A regression from #1033 Let's add back the two styles of triggers: - click: show mouse pointer to signal that it is clickable and use underline dashed - not click: no special cursor, and use underline dotted
A regression from MarkBind#1033 Let's add back the two styles of triggers: - click: show mouse pointer to signal that it is clickable and use underline dashed - not click: no special cursor, and use underline dotted
Fix a regression from #1033. Let's add back the two styles of triggers: - click: show mouse pointer to signal that it is clickable and use underline dashed - not click: no special cursor, and use underline dotted
Fix a regression from #1033. Let's add back the two styles of triggers: - click: show mouse pointer to signal that it is clickable and use underline dashed - not click: no special cursor, and use underline dotted
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [ ] Documentation update
• [ ] Bug fix
• [ ] New feature
• [x] Enhancement to an existing feature
• [ ] Other, please explain:
Fixes #873
Fixes #814
Updated PR, modals, tooltips, and popovers are all working
Some breaking changes:
Contextmenu (right click) no longer a trigger
bootstrap-vue modals don't support the width attribute
But their modals have a whole lot of other options, see them here:
https://bootstrap-vue.js.org/docs/components/modal/
Popovers now don't have the zoom effect. I tried to reimplement the zoom effect, but was only successful for modals. Will try to add back in later.
I have updated the docs for these.
What is the rationale for this request?
We are still reliant on vue-strap for popovers, modals, and tooltips. This aims to complete the migration to bootstrap-vue, by using their versions of tooltips, popovers, and modals.
What changes did you make? (Give an overview)
Updated our bootstrap-vue js and css to the latest version. This gives us better versions of popovers and tooltips.
bootstrap-vue's trigger mechanism is very different from ours:
Each popover must target a single element on page load, so our "many triggers for one popover" implementation does not work.
So, we take all
<trigger>
elements and turn it into a with a v-b-popover directive, and pass in functions that allow us to return a HTML string of the desired popover.Provide some example code that this change will affect:
Taken from the docs:
Old:
New:
Is there anything you'd like reviewers to focus on?
Functionality of the popovers.
Note that modals and tooltips are broken for now, but the solution for them should be similar
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)