Skip to content
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

Views hints #767

Merged
merged 18 commits into from
Oct 24, 2017
Merged

Views hints #767

merged 18 commits into from
Oct 24, 2017

Conversation

natanfelles
Copy link
Contributor

@natanfelles natanfelles commented Oct 10, 2017

Hi, @puschie286.

Sorry for I look ignorant in your last issue. I was a bit tired and confused. But I've worked a little bit on the issue and now it may be possible to disable the views with this PR.

I tried to surround the views with comments like:

<!-- start_view: VIEWPATH -->
View content...
<!-- stop_view -->

On button click change to something like:

<!-- start_view: VIEWPATH -->
<div class="debug-view"><div class="debug-view-path" style="display: none;">VIEWPATH</div>
View content...
</div><!-- stop_view -->

I spent about two hours there, but I could not handle the DOM.

@natanfelles
Copy link
Contributor Author

Can fix #761

@natanfelles
Copy link
Contributor Author

natanfelles commented Oct 10, 2017

The reason about the comments was that functions like insertBefore() do not worked. And if has comments out of the body was not possible to use replace() to change the data via innerHTML.

A way that was tried to get the comments and in a loop replace the contents on button click.

After a while trying this came the idea of disabling the views hints via commenting the App config and using the default Views collector. So there's PR.

Sorry, I was not merged with the latest updates.

@puschie286
Copy link
Contributor

puschie286 commented Oct 11, 2017

hey, np^^
these commits only handle the badges right ? ( was looking for comment replacement code^^ )

for the insert problem - i rewrote the toogleViewsHints function, its a bit more complex as i excepted and not much tested. hopefully you can get the idear behind this and extend it for special cases i missed

( currently i cant push, but as it is just one function i post the code here - sry for that )
output with comments

$output = '<!-- DEBUG-VIEW START ' . $view . '-->' . $output . '<!-- DEBUG-VIEW ENDED -->';
toogleViewsHints: function()
    {
		var NodeList 		= []; // [ Element, NewElement( 1 )/OldElement( 0 ) ]
		var sortedComments 	= [];

    	// get all comments | iterate over all nodes
		// @return array of comment nodes
    	var GetComments = function( parentElement )
		{
			var comments = [];
			var childs = parentElement.childNodes;
			for( var i = 0; i < childs.length; ++i )
			{
				if( childs[i].nodeType === Node.COMMENT_NODE &&
					childs[i].nodeValue.startsWith( ' DEBUG-VIEW' ) )
				{
					comments.push( childs[i] );
					continue;
				}

				if( childs[i].childNodes.length > 0 )
				{
					var childComments = GetComments( childs[i] );
					comments.push.apply( comments, childComments );
				}
			}
			return comments;
		};

    	// find node that has TargetNode as parentNode
    	var GetParentNode = function( Node, TargetNode )
		{
			if( Node.parentNode === null )
			{
				return null;
			}

			if( Node.parentNode !== TargetNode )
			{
				return GetParentNode( Node.parentNode, TargetNode );
			}

			return Node;
		};

    	// get next valid element ( to be safe to add divs )
		// @return element node
    	var GetValidElement = function( NodeElement )
		{
			if( NodeElement.nextElementSibling !== null )
			{
				// handle invalid tags : html, head, body, ...
				if( NodeElement.nextElementSibling.nodeName === "HTML" )
				{
					// inside body is valid | so return first
					return document.body.children[0];
				}

				return NodeElement.nextElementSibling;
			}
			else if( NodeElement.previousElementSibling !== null )
			{
				// TODO : handle special cases

				return NodeElement.previousElementSibling;
			}

			// something went wrong!
			return null;
		};

    	var comments = GetComments( document );
    	// sort comment by opening and closing tags
		for( var i = 0; i < comments.length; ++i )
		{
			// get file path + name to use as key
			var Path = comments[i].nodeValue.substring( 18, comments[i].nodeValue.length - 1 );

			if( comments[i].nodeValue[12] === 'S' ) // simple check for start comment
			{
				// create new entry
				sortedComments[Path] = [ comments[i], null ];
			}
			else
			{
				// add to existing entry
				sortedComments[Path][1] = comments[i];
			}
		}
		comments.length = 0;

        var btn = document.querySelector('[data-tab=ci-views]');

        // If the Views Collector is inactive stops here
        if (! btn)
        {
            return;
        }

        btn = btn.parentNode;

        btn.onclick = function() {
            // Had AJAX? Reset view blocks
			//comments = GetComments( document ); // TODO : remove comment/enable line

            if (ciDebugBar.readCookie('debug-view'))
            {
            	for( var i = 0; i < NodeList.length; ++i )
				{
					var index;

					// find index
					for( var j = 0; j < NodeList[i].parentNode.childNodes.length; ++j )
					{
						if( NodeList[i].parentNode.childNodes[j] === NodeList[i] )
						{
							index = j;
							break;
						}
					}

					// move child back
					for( var j = 1; j < NodeList[i].childNodes.length; ++j )
					{
						NodeList[i].parentNode.insertBefore( NodeList[i].childNodes[j], NodeList[i].parentNode.childNodes[index].nextSibling  );
					}

					NodeList[i].parentNode.removeChild( NodeList[i] );
				}
				NodeList.length = 0;

                ciDebugBar.createCookie('debug-view', '', -1);
                ciDebugBar.removeClass(btn, 'active');
            }
            else
            {
            	for( var key in sortedComments )
				{
					var StartElement 	= GetValidElement( sortedComments[key][0] );
					var EndElement 		= GetValidElement( sortedComments[key][1] );

					// find element which has same parent as startelement
					EndElement = GetParentNode( EndElement, StartElement.parentNode );
					if( EndElement === null )
					{
						// cant create divs if endelement is higher as startelement
						continue;
					}

					var DebugDiv 		= document.createElement( 'div' ); // holder
					var DebugPath		= document.createElement( 'div' ); // path
					var ChildArray 		= StartElement.parentNode.childNodes; // target child array
					var Parent			= StartElement.parentNode;
					var Start, End;

					// setup container
					DebugDiv.classList.add( 'debug-view' );
					DebugDiv.classList.add( 'show-view' );
					DebugPath.classList.add( 'debug-view-path' );
					DebugPath.innerText = key;
					DebugDiv.appendChild( DebugPath );

					// calc distance between them
					// Start
					for( var i = 0; i < ChildArray.length; ++i )
					{
						if( ChildArray[i] === StartElement )
						{
							Start = i;
							break;
						}
					}
					// End
					for( var i = Start; i < ChildArray.length; ++i )
					{
						if( ChildArray[i] === EndElement )
						{
							End = i;
							break;
						}
					}
					// add container to DOM
					NodeList.push( Parent.insertBefore( DebugDiv, ChildArray[Start] ) );

					// move elements
					var Number = Start - End + 1;
					for( var i = 0; i < Number; ++i )
					{
						DebugDiv.appendChild( ChildArray[Start] );
					}
				}

                ciDebugBar.createCookie('debug-view', 'show', 365);
                ciDebugBar.addClass(btn, 'active');
            }
        };
    },

currently there are still some bugs - i will continue tomorrow, so just to get an idear how i deal with it

@natanfelles
Copy link
Contributor Author

natanfelles commented Oct 11, 2017

This commits handle the Views badge, showing the count of views using the default Views collector. The views only will be surrounded with extra divs if CI_DEBUG is on, has an Filter after with the toolbar index and if the Views are active in the Toolbar Collector.

To solve problems with CSS or JavaScript selectors parent and child just comment the Toolbar Collector Views. Commenting this, the Label Views will not be showed, the views will not be surrounded with extra divs and the views statistics will not be showed in the Timeline Tab.

If someone do not use parents and children selectors then is not necessary disable it. Because the extra divs will not change the look of the page if the Label Views, in the Toolbar, is disabled.

Perhaps only surround the views with comments be a best idea because the developer will can see where a view starts and ends in the source code. But this do not provide a practical visual orientation.

If we can add only the comments but change it dynamically on Views label click means that will be best.

The problem is that I have not success trying handle HTML comments with javascript yet.

I will try more with your code... now. The coffee's already hot. 😃

Ah, something similar to this was done in Magento.

@natanfelles
Copy link
Contributor Author

Hopi, no success with javascript replacement of HTML comments to divs.

If elements are out of the body, in this case the comments always will be, the console always return error:

HierarchyRequestError: Node cannot be inserted at the specified point in the hierarchy

All the comments inside the body can be handled and then add custom divs to surround the views, but if has comments out, the browser do not permit do it. Then, the first view (or others out of the body) never can be surrounded via javascript.

I tried several times and did several searches and the conclusion I had is that it is not possible to manipulate the page structure outside the body. Then the divs must already be there for the hints work properly.

As we have discussed some js or css can break if an extra div of the hints is inserted between children and parent elements. But also, with this PR, there is the option of disabling the Views Collector simply by commenting on it.

@puschie286
Copy link
Contributor

thats why i wrote the GetValidElement function, which is looking for place inside the body if the comment is outside

@puschie286
Copy link
Contributor

puschie286 commented Oct 12, 2017

so i finished the work on toogleViewsHints - if you like i can push it to this pull request ( would need the permissions to ).
looks good for my test page and should be handle most situations. would be great if you could test it on your test pages
EDIT : if you would like to test it first : https://pastebin.com/xZpiR8qh

@natanfelles
Copy link
Contributor Author

Tested but no success.

TypeError: sortedComments[Path] is undefined

If you did it works, push.

natanfelles and others added 2 commits October 13, 2017 01:13
~ reimplement toogleviewhints for dynamic add/remove of tags on valid positions

Signed-off-by: Christoph Potas <christoph286@googlemail.com>
@puschie286
Copy link
Contributor

so, here is the pull request for your fork : #1 Dynamic Debug-Views onclick

puschie286 and others added 2 commits October 17, 2017 11:06
~ fix skipping end element if we want to include it to the view

Signed-off-by: Christoph Potas <christoph286@googlemail.com>
@lonnieezell
Copy link
Member

I just took a quick glance, but a couple of notes:

  1. our style guide requires variable names to be camelCase, not PascalCase.
  2. It looks like every node in the DOM tree is being traversed during toggleViewsHints. That could be fairly slow, I would think. How's the performance on that? It's obviously not truly crucial for this type of a process, but would using xpath to find the comments be more efficient? To be honest, I've only really started to use xpath at all recently, working on some of the testing tools, but I know most of the major javascript libraries are built on it.

@puschie286
Copy link
Contributor

i will check the xpath and change the names tomorrow at work.

i didnt notice any performance problems but didnt test it with a high view usage - so i will have an eye on it

Signed-off-by: Christoph Potas <christoph286@googlemail.com>
~ use finding on every click to react to ajax changes

Signed-off-by: Christoph Potas <christoph286@googlemail.com>
@puschie286
Copy link
Contributor

did it : #2 Use XPath search & renaming for style guide

xpath is a much faster ( so i implement it ) - but i havent test it on different browser
pls check this jsperf

@lonnieezell
Copy link
Member

Wow. 10x increase in performance. I expected it to be faster, but not that much. That's awesome. Thanks!

@jaynarayan89
Copy link
Contributor

@natanfelles This feature is preety cool. Thanks

@lonnieezell
Copy link
Member

@natanfelles are you done with this PR?

@natanfelles
Copy link
Contributor Author

Yes. I did some tests with the improvements of @puschie286 and is working nice (on Firefox and Chrome) here.

@lonnieezell
Copy link
Member

Awesome. Merging. Thanks!

@lonnieezell lonnieezell merged commit d072ec9 into codeigniter4:develop Oct 24, 2017
@natanfelles natanfelles deleted the views_hints branch October 31, 2017 21:28
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.

4 participants