Add note on absolute links #2507

jingting1412 · 2024-04-08T05:07:56Z

What is the purpose of this pull request?

Overview of changes:
#9 and #278

When including contents with links, {{baseUrl}} is required to be added to the link if the user wants it to point to the absolute location. Without this note, some users might not realise this can result in broken links as shown in issue #278

Anything you'd like to highlight/discuss:

Testing instructions:
markbind serve -d the docs the see if the warning message under the includes section is clear.

Proposed commit message: (wrap lines at 72 characters)
Add note on URLs in includes

Checklist: ☑️

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

Reviewer checklist:

Indicate the SEMVER impact of the PR:

Major (when you make incompatible API changes)
Minor (when you add functionality in a backward compatible manner)
Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

what was the old feature that was made obsolete

any replacement feature (if any), and

how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

yucheng11122017

Hi @jingting1412 thanks for the work!

I think this issue about using baseUrl isn't just applicable to includes. It also affects panels, modals etc which use links.

Could you update those areas also? Gentle reminder to use includes to avoid duplication.

Would be also good to add it to troubleshooting section in our user guide

jingting1412 · 2024-04-09T16:19:24Z

Hi @jingting1412 thanks for the work!

I think this issue about using baseUrl isn't just applicable to includes. It also affects panels, modals etc which use links.

Could you update those areas also? Gentle reminder to use includes to avoid duplication.

Would be also good to add it to troubleshooting section in our user guide

Yep for sure! I've added the tip in more places where I think its relevant and the troubleshooting section. Do let me know if I need to refine my explanation or add the tip to more places.

yucheng11122017

Hi @jingting1412 thanks for the work! Some nits :)

docs/userGuide/syntax/includes.md

yucheng11122017 · 2024-04-11T09:07:14Z

docs/userGuide/syntax/includes.md

+<div id="baseUrl-warning">
+<box type="warning" header="Add `{{ '{{ baseUrl }}' }}` to local URLs to always point to the same target!">
+
+For links within `<include>` to always point to the same target, make it an absolute link with `{{ '{{ baseUrl }}' }}` included.  


Could you include a negative example so users know what what not to do as well?

yep sure included! lmk if its not clear or anything

docs/userGuide/syntax/includes.md

yucheng11122017 · 2024-04-11T09:08:15Z

docs/userGuide/troubleshooting.md

+To solve this, change the relative URL into an **absolute** URL by adding `{{ '{{ baseUrl }}' }}`. This will ensure that the link will always point to the same address regardless of the page it is included in.
+
+###### Example: Adding `{{ '{{ baseUrl }}' }}` to make absolute URLs


Can we use a box here instead?

yep sure I have included it now!

yucheng11122017 · 2024-04-11T09:08:40Z

docs/userGuide/troubleshooting.md

+###### Example: Adding `{{ '{{ baseUrl }}' }}` to make absolute URLs
+
+If file `folder1/file1.md` contains an absolute link `{{ '{{ [link]({{ baseUrl }}/folder1/target.html) }} ' }}`, when `file1.md` is included in `folder2/file2.md`, the link will point to `folder1/target1.html` even if the target is not in the same directory as `file2.md`.


This example is the same as other parts. Could we use includes to reduce duplication?

yep sure, the example can now be included using includes

Since we are using include from the include section, I was thinking whether we should create one that is more generic since user may be confused by the mentions of include? Wdyt? @jingting1412 @yucheng11122017

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

yucheng11122017

Hi @jingting1412 thanks for the work! I think the formatting and so on is good.

I would prefer a more concrete example for the example we provide though (see suggestions). What do you think!

docs/userGuide/troubleshooting.md

docs/userGuide/syntax/includes.md

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

kaixin-hc

Hmm, not so sure on the look of the box.

Based off of @EltonGohJH 's comments, I added some suggestions on how to rephrase it so it sounds a bit more generic...

kaixin-hc · 2024-04-12T12:36:42Z

docs/userGuide/syntax/popovers.md

@@ -71,6 +71,8 @@
 </variable>
 </include>

+<include src="includes.md#baseUrl-warning"/>


When embedded, it seems like the warning is very large and disrupts the flow of the text. Actually, even when it's not included the placing feels a little awkward. What if all or some of the content was in an expandable panel, so that it's out of the way? Just the header might be good? I suppose you could also experiment with having the example be expandable instead.

yep sure, I've put the example into a panel and changed the explanation to make it more clear! I think since this issue is about links breaking when being included with <include>, the note at the includes section in the UG and troubleshooting section is also just about this specific problem.

If there's another cause to the link breaking issue other than include I think we can make a more generic one?

docs/userGuide/syntax/includes.md

yucheng11122017

LGTM except for some nits :)

docs/userGuide/syntax/includes.md

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

kaixin-hc · 2024-04-14T16:09:48Z

@jingting1412 hi, refer to this image for what i mean about weird spacing... could you add a new line where it makes sense to avoid this kind of 0 margin case?

And this warning is redundant.

Thanks!

jingting1412 · 2024-04-15T03:14:37Z

@jingting1412 hi, refer to this image for what i mean about weird spacing... could you add a new line where it makes sense to avoid this kind of 0 margin case?
And this warning is redundant. Thanks!

Hi thanks for the spot! I've fixed those areas now

yucheng11122017

LGTM!

Tim-Siu

The added documentation looks very clear. The only complaint is that the border between the panel and the box boundary is slightly wider than what I prefer, but that is not the problem with this PR.

github-actions · 2024-04-16T15:19:43Z

@Tim-Siu Each PR must have a SEMVER impact label, please remember to label the PR properly.

jingting1412 added 2 commits April 8, 2024 13:03

Add note on absolute links

80be602

Fix typos

6e3617b

yucheng11122017 reviewed Apr 9, 2024

View reviewed changes

jingting1412 and others added 2 commits April 9, 2024 20:11

Merge branch 'MarkBind:master' into popup-docs

92000fe

Add tip to more pages

37dae13

yucheng11122017 reviewed Apr 11, 2024

View reviewed changes

jingting1412 and others added 5 commits April 11, 2024 17:16

Merge branch 'MarkBind:master' into popup-docs

337ec36

Update docs/userGuide/syntax/includes.md

fc0e857

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Update docs/userGuide/syntax/includes.md

7f25bb9

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Add negative example

c9f1a47

Merge branch 'MarkBind:master' into popup-docs

cf4251f

yucheng11122017 reviewed Apr 12, 2024

View reviewed changes

docs/userGuide/troubleshooting.md Outdated Show resolved Hide resolved

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

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

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

jingting1412 and others added 6 commits April 12, 2024 17:47

Merge branch 'MarkBind:master' into popup-docs

ac5bc63

Update docs/userGuide/troubleshooting.md

0fd7aa1

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Update docs/userGuide/syntax/includes.md

5a2bb07

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Update docs/userGuide/syntax/includes.md

1f207ee

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Update docs/userGuide/syntax/includes.md

cdd953c

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Add more details

ed502e8

kaixin-hc reviewed Apr 12, 2024

View reviewed changes

Improve explanation

686e19b

yucheng11122017 approved these changes Apr 12, 2024

View reviewed changes

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

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

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

jingting1412 and others added 5 commits April 13, 2024 00:15

Merge branch 'MarkBind:master' into popup-docs

ced6b4d

Improve look of tip

edfd5b0

Update docs/userGuide/syntax/includes.md

9dff565

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Update docs/userGuide/syntax/includes.md

263eacd

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Update docs/userGuide/syntax/includes.md

1df6f67

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

jingting1412 and others added 2 commits April 15, 2024 10:57

Merge branch 'MarkBind:master' into popup-docs

2d9d4f4

Implement review changes

262c987

yucheng11122017 approved these changes Apr 16, 2024

View reviewed changes

Merge branch 'master' into popup-docs

1693d54

Tim-Siu approved these changes Apr 16, 2024

View reviewed changes

Tim-Siu merged commit fc776ba into MarkBind:master Apr 16, 2024
7 checks passed

Tim-Siu added c.Enhancement p.Low a-Documentation 📝 pr.DocsUpdate 📃 Pure changes to the documentation, such as typo, restructuring, etc d.moderate r.Patch Version resolver: increment by 0.0.1 labels Apr 16, 2024

kaixin-hc removed the c.Enhancement label Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add note on absolute links #2507

Add note on absolute links #2507

jingting1412 commented Apr 8, 2024 •

edited

yucheng11122017 left a comment

jingting1412 commented Apr 9, 2024

yucheng11122017 left a comment

yucheng11122017 Apr 11, 2024

jingting1412 Apr 11, 2024

yucheng11122017 Apr 11, 2024

jingting1412 Apr 11, 2024

yucheng11122017 Apr 11, 2024

jingting1412 Apr 11, 2024

EltonGohJH Apr 11, 2024

yucheng11122017 left a comment

kaixin-hc left a comment

kaixin-hc Apr 12, 2024

jingting1412 Apr 12, 2024 •

edited

yucheng11122017 left a comment

kaixin-hc commented Apr 14, 2024

jingting1412 commented Apr 15, 2024

yucheng11122017 left a comment

Tim-Siu left a comment

github-actions bot commented Apr 16, 2024

		To solve this, change the relative URL into an absolute URL by adding `{{ '{{ baseUrl }}' }}`. This will ensure that the link will always point to the same address regardless of the page it is included in.

		###### Example: Adding `{{ '{{ baseUrl }}' }}` to make absolute URLs

		###### Example: Adding `{{ '{{ baseUrl }}' }}` to make absolute URLs

		If file `folder1/file1.md` contains an absolute link `{{ '{{ [link]({{ baseUrl }}/folder1/target.html) }} ' }}`, when `file1.md` is included in `folder2/file2.md`, the link will point to `folder1/target1.html` even if the target is not in the same directory as `file2.md`.

Add note on absolute links #2507

Add note on absolute links #2507

Conversation

jingting1412 commented Apr 8, 2024 • edited

yucheng11122017 left a comment

Choose a reason for hiding this comment

jingting1412 commented Apr 9, 2024

yucheng11122017 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucheng11122017 left a comment

Choose a reason for hiding this comment

kaixin-hc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingting1412 Apr 12, 2024 • edited

Choose a reason for hiding this comment

yucheng11122017 left a comment

Choose a reason for hiding this comment

kaixin-hc commented Apr 14, 2024

jingting1412 commented Apr 15, 2024

yucheng11122017 left a comment

Choose a reason for hiding this comment

Tim-Siu left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 16, 2024

jingting1412 commented Apr 8, 2024 •

edited

jingting1412 Apr 12, 2024 •

edited