Documentation Sidebar Links Href Incorrect
Closed this issue · 20 comments
When viewing documentation, the sidebar links href is set to "#" which prevents ctrl + clicking to open in a new tab correctly.
This happens on any of the subpages within the docs.
Example page: https://docs.dbatools.io/Repair-DbaDbOrphanUser
hum, @brandon-hurler this was already reported but was fixed ... and my ctrl+click works ??? you get the correct href just before pressing the button
before
clicked or ctrl+clicked
and it opens a new tab
just to know, what browser are you using ? I'm on chrome and edge.
BTW, this piece is on all pages and should work the same on all of them
https://github.com/dataplat/docs/blob/master/Add-DbaAgDatabase.html#L263
Ah, I see what the issue is. I was mistaken on the ctrl + click. I am using the middle mouse button to open in a new tab. Normally clicking your scroll wheel is the same functionality as ctrl + click, but if ctrl + click is being manually handled for instead of normally href'ed, then it would slip through.
So it looks like clicking the scroll wheel still needs to be updated.
Also, right-clicking on the link and clicking "Open in new tab" or "Open in new window" don't work either.
Manually handling for the link opening coops built in browser navigation. This may be an overall routing issue that needs to be looked into?
Edit: I am using Brave as my browser which is chromium like Chrome or Edge. However, I have replicated this issue in the other browsers as well.
so basically we could create the link on hover rather than on click, but at least now I understand what the problem is :D
as for "overall routing issue" .... I don't get the point, but that's prolly me being a non-native english speaker.
Ah, no worries!
When the DOM elements are generated, the href is generated as "#". Rather than handling those with JavaScript manually for different functions and replacing the # with the name on click, you should set the href directly.
This allows consistent behavior across browsers and you can remove the JavaScript that is manually handling for the on click events.
In the screenshots below, I also removed the nested span since the DOM can be simplified with the CSS remaining the same.
Here's some screenshots of what I mean:
One additional note, the contents of the anchor tag and the href can be set on page load instead of with JavaScript. Whichever way the <span class="CommandName">Add-DbaExtendedProperty</span> is being set.
Edit: formatting
you're failing to consider that the menu gets built by javascript, and once the link gets created when hovered, you should be able to navigate with whatever browser and key-combination you like ;-) but if you absolutely need for some reason for allllll links to be filled even without ever being clicked ..... please do tell.
Even if the page is populated by JavaScript, say for a React application, you can still set the anchor tags correctly on page load.
it's a simple piece of javascript without bundling heavy js frontends (list.js the thingy we use to build the menu). Once again, I get the consideration of "please put the href in on hover rather than on click because if not middle-mouse-click doesn't work" but I don't get the "please fill all of them no matter what".
I am genuinely trying to help your application have a better user experience. Not having those populated is breaking browser convention, causing bugs, and it isn't accessibility friendly. If someone is using accessibility software to navigate, it will likely not work with your current implementation.
I looked through the code and found the lines you would need to change.
accessibility is a perfect reason, now I get it !
Awesome. I hope the feedback has been useful and can help resolve the issue.
I have a separate question. Are the HTML files in this repo automatically generated, or do you have to change this in each file?
yep @brandon-hurler , superuseful, I wasn't trying to be nitpicky, just genuine curiosity on what was the difference of having the href ready when hovered upon on demand rather all of them to be set no matter what. And accessibility is a perfect reason I didn't think about.
As for generation, yeah, there's two templates (assets/templates), one for the index and one for the pages that have more or less the same code... then there's an action that rebuilds the pages injecting the markdown-generated-from-inline-help into html, so we get good results from search engines.
I assumed, just wanted to be sure. If you can fix it within the cmdlist, then all native browser functionality will automatically work and it will make it easier to maintain :)
it's actually not that easy because the templating in list.js doesn't allow your suggested fix but I'll manage to get it working in the next few hours ;-)
No worries, I don't have the code pulled so I am just doing it by sight. I more or less meant it will be easier to fix in one place than having to separately handle for all the different browser navigation functions. Thank you for looking into this!
hi @brandon-hurler , I pushed a version that should work fine. However dbatools is in the process of reorganizing the module structure so in order to push the pages @potatoqualitee needs to update the pipeline that trigger the rebuild of the documentation to point to the "newer" correct structure. Stay tuned ;-)
oof, something went wrong after i updated the files https://github.com/dataplat/docs/actions/runs/3706455890/jobs/6293933668
Looks like one of the lines in dbatools.psd1 may not have a path correctly wrapped in single or double quotes and it is interpreting a path as a command.
i think it may be the way im testing for the version of the library it needs -- there's new code in the psd1 to check if its core or not, but i havent yet had time to load it all up and test
this is now fixed