Skip to content

Conversation

@jamos-tay
Copy link
Contributor

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Bug fix

Fixes #442

What is the rationale for this request?

Anchors were not being generated for panel headers, and in a previous version do not redirect to the page when clicked.

What changes did you make? (Give an overview)

Anchors should now be generated for panel headers correctly.

The panel headers are stored in the panel's header attribute and rendered by vue, so headings weren't being picked up by Page. I worked around this by checking all panels for the header attribute and adding the anchor html there.

As for the links not working previously (see #442 (comment)), I added some code to setup.js to manually redirect the page when an anchor is clicked, it seems to be working as expected now.

@jamos-tay jamos-tay force-pushed the anchor-in-panel branch 3 times, most recently from e45d2b9 to c002fc2 Compare October 15, 2018 12:35
@jamos-tay
Copy link
Contributor Author

jamos-tay commented Oct 15, 2018

The tests are passing locally on my machine (windows) but for some reason Travis seems to be failing.

Somehow when rendered on Travis the block of code I added causes the src attribute of panels to change, all the \ are converted to /. Here's a diff (expected | actual):

image

Could I get some help on this?

}

if (isDiff(part) && !insidePath) {
if (isDiff(part) && !insidePath && !isPathSeparatorDiff(part.value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The change here allows for this situation to pass:
expected.html

<div> \ </div>

actual.html

<div> / <div>

@nicholaschuayunzhi
Copy link
Contributor

nicholaschuayunzhi commented Oct 16, 2018

Somehow when rendered on Travis the block of code I added causes the src attribute of panels to change, all the \ are converted to /.

The reason is because Travis is running a unix env and the path separator generated will be / whereas on Windows, it will be \.

As for the diff that is being shown, I would think my diffing code will account for that and not trigger an error. Sorry about that 🙏. I will help to look into the matter.

Also see my comment about your fix!

@nicholaschuayunzhi
Copy link
Contributor

I found the bug:

its found in endsWithOpeningTag in diffHTML

The fragment that causes this is (note i've formatted it for readability)

<panel header="## Panel with src from another Markbind site header<a class='fa fa-anchor' href='#panel-with-src-from-another-markbind-site-header'></a>" 
src="/test_site
+/
-\
sub_site/index._include_.html" expanded=""></panel>

To decide if the diff \ is found in a path, it checks if the diff

  1. is within a path - checking if there is a preceding src=" (yes)
  2. fragment is within a html tag
    • here i check if i find a > or < first.
      • if < is found its within a html tag - diff is in path
      • if > is found assume not within html tag - diff is not in path (and this is incorrect for the case above)

Would a good solution be to check the parity of the < and > ?

@jamos-tay
Copy link
Contributor Author

jamos-tay commented Oct 17, 2018

@nicholaschuayunzhi Nice! I'll go ahead and remove that line. Will wait for your PR to be merged first.

@nicholaschuayunzhi
Copy link
Contributor

@jamos-tay The fix has been merged to master, you can go ahead and rebase on master and also drop commits d6e6851 and eb2f83a

@jamos-tay
Copy link
Contributor Author

Sure, updated 👍

@yamgent
Copy link
Member

yamgent commented Oct 20, 2018

Seems OK, but user-usability wise, I actually expected the anchor to have appeared when the mouse is inside the panel's header box, but as of now you must hover the mouse inside the header text in order to reveal it.

anchor

@jamos-tay
Copy link
Contributor Author

Yeah... the dimensions of the header seems to be different for headers in panels.

I'll try and find a fix

@jamos-tay
Copy link
Contributor Author

@yamgent Hey, think it should be fixed now.

There's still a minor nit where the anchor will disappear if the user hovers over the header text first, then moves the mouse right until the empty space (basically mouseleave gets triggered for the header), but fixing it would be quite troublesome and I think it's small enough to ignore.

@yamgent yamgent self-requested a review October 31, 2018 03:04
Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Awesome 👍

Just one minor nit left:

@jamos-tay
Copy link
Contributor Author

Changes made

@yamgent yamgent added this to the v1.14.1 milestone Nov 6, 2018
@yamgent yamgent merged commit 91cdc98 into MarkBind:master Nov 6, 2018
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.

Show anchor icons for heading inside panels

3 participants