Implement src attribute for popovers #1780

jovyntls · 2022-02-16T16:19:31Z

What is the purpose of this pull request?

Resolves #59

Overview of changes:
Implements the src attribute in popovers, which allows popover contents to be loaded from other files.

Anything you'd like to highlight / discuss:

Static vs dynamic approach
- See discussion on Allow src attribute for popovers #59 on using a static approach (similar to Includes) vs dynamic approach (similar to Panels)
Behaviour on conflicting slots/attributes
- Currently, popover contents can be given through 3 ways. I used the following 'priority' of overriding:
  1. content slot
  2. src attribute
  3. content attribute
- Rationale: For other MarkBind components, slots are taken as the highest 'priority' and an attribute will be overwritten/ignored if the corresponding slot is provided. Though I'm not sure if src attribute should override content slots in this case?

Testing instructions:

<popover header="Content loaded from src" src="{{ baseUrl }}/userGuide/syntax/extra/loadContent.html#fragment">
	Hover
</popover>

Proposed commit message: (wrap lines at 72 characters)

Implement src attribute for popovers

Content can be supplied to popovers through either the content slot, or
content attribute.

Manually providing popover contents may be inconvenient if an author
wants to reference content from elsewhere on the site. 

Let's implement the src attribute for popovers, so that authors can
reference content from source files. This is more convenient for
authors, while helping to ensure consistency in website contents.

Using the src attribute maintains consistency with other components such
as Panels and Includes, which also use src.

The src attributed was implemented with a static approach to avoid
potential positioning issues. In the case that multiple contents are
supplied, the following priority is used: content slot > content
attribute > src attribute

Checklist: ☑️

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

tlylt

Great job!
One slight clarification:
Does the src allows for both a html page URL and a markdown file (just like the include mechanism)?
If so perhaps we could mention both and give one example each.

Also there's a conflict due to the latest PR merge.

PS. one more question in mind:
Just curious, does the importing of content from an external URL leads to a potential XSS attack, or has it already been handled accordingly?

jovyntls · 2022-02-19T10:55:39Z

@tlylt Thanks for looking through!

Great job! One slight clarification: Does the src allows for both a html page URL and a markdown file (just like the include mechanism)? If so perhaps we could mention both and give one example each.

Yup! Good idea, I'll add it to the docs

Also there's a conflict due to the latest PR merge.

Fixed, thanks for the catch :)

PS. one more question in mind: Just curious, does the importing of content from an external URL leads to a potential XSS attack, or has it already been handled accordingly?

By external URL, do you mean something like src="{{baseUrl}}/hello.html"? For this, XSS is possible though there's currently no warning given for users. I'm not sure if this will be a threat though, since these files are usually supplied by the user themselves?

tlylt · 2022-02-19T12:05:21Z

By external URL, do you mean something like src="{{baseUrl}}/hello.html"? For this, XSS is possible though there's currently no warning given for users. I'm not sure if this will be a threat though, since these files are usually supplied by the user themselves?

Ok, I think I had the wrong impression that the src could be any URL to an HTML page on the web but I think it doesn't seem to be the case. Hence maybe it's not that necessary to give any warnings at the moment.

ryoarmanda · 2022-02-20T17:04:45Z

Thanks for the work, @jovyntls ! Review is underway.

Currently, popover contents can be given through 3 ways. I used the following 'priority' of overriding:
a. content slot
b. src attribute
c. content attribute

Rationale: For other MarkBind components, slots are taken as the highest 'priority' and an attribute will be overwritten/ignored if the corresponding slot is provided. Though I'm not sure if src attribute should override content slots in this case?

Let's discuss about the priority list. Just my 2 cents, how about if we put src attribute last? This is under the assumption that users would normally do just one of these three, which is a reasonable use-case and what is exampled in the issue.

It's a little bit unintuitive to think src will be overridden by content, but I like to think that just like a general scoping rule, the closest specified content should "win". In this case, it means content that is supplied directly when writing the popover component, which is either through the content attribute or the content slot (we can follow suit with other components and let the slot win if both are defined). The content specified in src is an externally-defined content wrt. the page and "loses" to those. If the user wishes to display that, however, they can just not write the content stuff (I guess it's a reasonable ask). What do you think?

jovyntls · 2022-02-23T17:06:38Z

@ryoarmanda
I like the idea of using "scoping" here - I think while src overriding content is more intuitive, using some sort of rule (scoping) would be better than the intuitive approach in case we need to standardise behaviour in the future.

ryoarmanda

Okay then, we can set the priority as discussed. In the meantime, here are some improvements that can be made for your implementation (the tests and docs reviews are to be done later when the implementation is solid). Feel free to address this alongside modifying the priority :)

P.S. don't forget to sync with master!

packages/core/src/html/includePanelProcessor.js

ryoarmanda · 2022-02-24T14:14:28Z

packages/core/src/html/includePanelProcessor.js

+        + `Missing reference in ${context.cwf}`);
+      logger.error(error);
+
+      actualContent = cheerio.html(createErrorNode(node, error));


This way, the error node I think would be in the tooltip content 😄 (let me know if this is correct and/or intended). If not, we can adopt the way to replace the node with error node like in the above lines and early return.

I used the same behaviour as include - i.e., when the src is invalid, it displays the error node:

Though on thinking further, I think it's a better idea to log the error, and not display the node instead 😅
Is this behaviour desirable for include too?

packages/core/src/html/MdAttributeRenderer.js

jovyntls · 2022-02-28T03:46:18Z

Thank you so much for the detailed look-through @ryoarmanda ! I've made the changes and re-synced with master.

ryoarmanda

Thanks for the follow up! Here are some reviews that we can work on:

packages/core/src/html/elements.js

packages/core/src/html/vueSlotSyntaxProcessor.js

ryoarmanda

Thanks for addressing the review! The implementation is pretty solid (with one very minor change below) and all that's left is coming up with the tests and cleaning up the documentation 😄

packages/core/src/html/includePanelProcessor.js

docs/userGuide/syntax/popovers.md

packages/cli/test/functional/test_site/testPopovers.md

docs/userGuide/syntax/popovers.md

…into popover-src-attribute

jovyntls · 2022-03-06T16:18:57Z

Thanks again @ryoarmanda for the detailed lookthrough! I've made the changes mentioned, and also made an additional change to packages/vue-components/README.md since I realised it hadn't been updated yet.

tlylt · 2022-03-07T00:40:15Z

Just checking, does this part in coreesrc/html/linkProcessor.js need to be updated as well?

jovyntls · 2022-03-07T07:22:42Z

@tlylt Thanks for the catch!

You're right, defaultTagLinkMap is used in convertRelativeLinks so popovers should be included as well, otherwise relative links won't work for the src attribute. I originally thought it wasn't needed since validateIntraLink returns immediately, didn't realise that convertRelativeLinks uses this as well.

ryoarmanda

Just some little documentation nits before ready to approve!

docs/userGuide/syntax/popovers.md

packages/vue-components/README.md

ryoarmanda

LGTM! 🎉

ryoarmanda · 2022-03-10T07:15:35Z

Hi @jovyntls , before I merge the changes, let's add more information to the commit message, as this is a considerable popover enhancement.

jovyntls · 2022-03-10T13:41:48Z

Thanks for the reminder @ryoarmanda ! I've updated the commit message to include more information.

jovyntls added 8 commits February 16, 2022 09:43

Implement src attribute

d1e9ce8

Update tests

5b98e15

Update docs

62123b6

Lint

d9e490a

Fix tests

315b67f

possibly cleaner?

8cd21f8

Refactor

4ba70d1

Fix typo

5e45e5e

jovyntls marked this pull request as ready for review February 17, 2022 00:41

tlylt reviewed Feb 18, 2022

View reviewed changes

jonahtanjz requested a review from ryoarmanda February 18, 2022 03:57

Merge branch 'master' into popover-src-attribute

ba79e24

Add HTML example to docs

fd2dc1a

ryoarmanda requested changes Feb 24, 2022

View reviewed changes

jovyntls added 5 commits February 28, 2022 11:16

Refactoring

de9255f

Fix priority of src and content attribs

05d0d65

Merge branch 'master' into popover-src-attribute

f4e4cab

Linting

0469ec0

Update tests after merge

5605e36

ryoarmanda reviewed Mar 3, 2022

View reviewed changes

packages/core/src/html/elements.js Outdated Show resolved Hide resolved

packages/core/src/html/vueSlotSyntaxProcessor.js Outdated Show resolved Hide resolved

jovyntls added 3 commits March 4, 2022 11:09

Fix slot template functions

8d9a4c5

Linting

12f5e95

Fix typo

aba09ec

ryoarmanda reviewed Mar 6, 2022

View reviewed changes

update README.md for vue-components

0ffe8dc

jovyntls added 5 commits March 6, 2022 21:00

Fix docs

a561d53

Merge branch 'popover-src-attribute' of github.com:jovyntls/markbind …

639bc19

…into popover-src-attribute

Update tests

beb449a

Add test cases

8836d0d

Revert unnecessary changes

a5ee754

jovyntls added 2 commits March 7, 2022 15:26

Add popovers to defaultTagLinkMap

d3c6b5d

Update tests for relative links

adbcfa4

ryoarmanda reviewed Mar 8, 2022

View reviewed changes

docs/userGuide/syntax/popovers.md Outdated Show resolved Hide resolved

packages/vue-components/README.md Outdated Show resolved Hide resolved

Documentation fixes

ab6e3d1

ryoarmanda approved these changes Mar 9, 2022

View reviewed changes

ryoarmanda added this to the 4.0 milestone Mar 9, 2022

ryoarmanda merged commit d30db7f into MarkBind:master Mar 10, 2022

This was referenced Mar 11, 2022

Refactoring for includePanelProcessor and MdAttributeRenderer #1817

Closed

Update pop-up docs #1832

Merged

This was referenced Dec 11, 2022

Inconsistent logger warning for repeated slots/attributes #1764

Closed

Include repeated attributes warning for panel elements #2053

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement src attribute for popovers #1780

Implement src attribute for popovers #1780

jovyntls commented Feb 16, 2022 •

edited

tlylt left a comment •

edited

jovyntls commented Feb 19, 2022 •

edited

tlylt commented Feb 19, 2022

ryoarmanda commented Feb 20, 2022 •

edited

jovyntls commented Feb 23, 2022

ryoarmanda left a comment •

edited

ryoarmanda Feb 24, 2022 •

edited

jovyntls Feb 28, 2022

jovyntls commented Feb 28, 2022

ryoarmanda left a comment

ryoarmanda left a comment

jovyntls commented Mar 6, 2022

tlylt commented Mar 7, 2022

jovyntls commented Mar 7, 2022

ryoarmanda left a comment

ryoarmanda left a comment

ryoarmanda commented Mar 10, 2022

jovyntls commented Mar 10, 2022

Implement src attribute for popovers #1780

Implement src attribute for popovers #1780

Conversation

jovyntls commented Feb 16, 2022 • edited

tlylt left a comment • edited

Choose a reason for hiding this comment

jovyntls commented Feb 19, 2022 • edited

tlylt commented Feb 19, 2022

ryoarmanda commented Feb 20, 2022 • edited

jovyntls commented Feb 23, 2022

ryoarmanda left a comment • edited

Choose a reason for hiding this comment

ryoarmanda Feb 24, 2022 • edited

Choose a reason for hiding this comment

jovyntls Feb 28, 2022

Choose a reason for hiding this comment

jovyntls commented Feb 28, 2022

ryoarmanda left a comment

Choose a reason for hiding this comment

ryoarmanda left a comment

Choose a reason for hiding this comment

jovyntls commented Mar 6, 2022

tlylt commented Mar 7, 2022

jovyntls commented Mar 7, 2022

ryoarmanda left a comment

Choose a reason for hiding this comment

ryoarmanda left a comment

Choose a reason for hiding this comment

ryoarmanda commented Mar 10, 2022

jovyntls commented Mar 10, 2022

jovyntls commented Feb 16, 2022 •

edited

tlylt left a comment •

edited

jovyntls commented Feb 19, 2022 •

edited

ryoarmanda commented Feb 20, 2022 •

edited

ryoarmanda left a comment •

edited

ryoarmanda Feb 24, 2022 •

edited