Fix redraw flashes at the JavaScript example roulette #1775

wilzbach · 2017-06-27T07:37:09Z

"Meh" solution instead of #1757

dlang-bot · 2017-06-27T07:37:10Z

Thanks for your pull request, @wilzbach! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

CyberShadow · 2017-06-27T07:38:42Z

index.dd

 )
-)))
+<script src="$(ROOT_DIR)js/example_roulette.js"></script>


Inline the script please?

I still would like to add the example selection as well (so the file will grow) and doing Js in Ddoc is really a pain.
Didn't you believe me, when I showed that using a separate file doesn't matter performance-wise?

I still would like to add the example selection as well (so the file will grow)

I don't understand how adding the dropdown should affect this part. The dropdown can only work when JavaScript is enabled, so it would need to be added at the end, no?

Didn't you believe me, when I showed that using a separate file doesn't matter performance-wise?

It doesn't matter today

It doesn't matter in that browser

Not an excuse to make a sub-ideal situation worse. What that shows is that more things need to be loaded lazily.

The dropdown can only work when JavaScript is enabled, so it would need to be added at the end, no?

No it should be done here as well - same motivation: preventing redraws

I think we need to worry about relayouts first; redraws shouldn't be much of a problem as long as they aren't too distracting. Avoiding relayouts should be easy by reserving space for the interactive stuff that will be filled in later by JS.

As for redraws (I assume you mean pop-in when new UI elements are displayed), we have too much stuff redrawing anyway with all the widgets etc. Not sure if it's worth complicating the implementation much just to be able to have the drop-down there from the start, though even so, I think it would be like 1 more line to select the right option, right? And the buttons are going to be added in later either way, right?

CyberShadow · 2017-06-27T07:40:00Z

js/example_roulette.js

+var rouletteChildNode = findFirst(rouletteChild.children, function(e) { return e.className.indexOf("runnable-example") != -1; });
+// step 2: select the first <pre> element (of the .runnable-example <div>)
+var el = findFirst(rouletteChildNode.children, function(e) { return e.nodeName == "PRE";});
+el.style.marginBottom = "20px";


BTW, this is not necessary. You can use the have-javascript class and do it with a CSS rule.

I need to reset this after the entire body has been parsed (and the JS for the building the examples layout gets triggered), not on start:

<body id='Home' class='doc'> <script type="text/javascript">document.body.className += ' have-javascript'</script>

Isn't this how a browser behaves?

Page gets fetched

browser fetches all scripts in the header

it starts evaluating the body -> adds the class

it hits the examples and executes this js (with the marginBottom hack)

it fetches all the scripts and stuff before and at the page bottom

it reaches the end of the page and reverts the marginBottom hack

Oh, I understand what you mean now. Just because you need to reset it doesn't mean you have to add it via JS too - JS styles override CSS ones because they're going to be set on the element itself. Regardless I agree that undoing/resetting it is suboptimal, see my comment below.

CyberShadow · 2017-06-27T10:28:37Z

js/run.js

@@ -127,6 +127,8 @@ $(document).ready(function()
    {
        var root = $(this);
        var el = root.children("pre");
+        // hack from the front page to avoid redraw flashes due to the buttons showing up
+        el[0].style.marginBottom = "0px";


Instead of removing the margin, can this be left in? As mentioned in the previous PR, my suggestion was to simply create a container (whose size is not going to change) for the buttons to be placed in. I think that would avoid a relayout.

wilzbach · 2017-06-27T19:17:04Z

As for redraws (I assume you mean pop-in when new UI elements are displayed), we have too much stuff redrawing anyway with all the widgets etc. Not sure if it's worth complicating the implementation much just to be able to have the drop-down there from the start, though even so, I think it would be like 1 more line to select the right option, right? And the buttons are going to be added in later either way, right?
...
Instead of removing the margin, can this be left in? As mentioned in the previous PR, my suggestion was to simply create a container (whose size is not going to change) for the buttons to be placed in. I think that would avoid a relayout.

Okay, I have removed the marginBottom hack and the code now contains only what's necessary to immediately select an example and shouldn't be controversial anymore.
Regarding the buttons: I won't be going for the container hack as imho the proper solution is to do the entire layout at once and not splitting it into pieces, but after this PR anyone can go down this road ;-)

CyberShadow · 2017-06-27T19:20:13Z

index.dd

+    var rouletteChild = examples[rouletteIndex];
+
+    // step 1: select the .runnable-example <div>
+    var rouletteChildNode = findFirst(rouletteChild.children, function(e) { return e.className.indexOf("runnable-example") != -1; });


Why not rouletteChild.getElementsByClassName?

CyberShadow · 2017-06-27T19:22:04Z

index.dd

+    // step 1: select the .runnable-example <div>
+    var rouletteChildNode = findFirst(rouletteChild.children, function(e) { return e.className.indexOf("runnable-example") != -1; });
+    // step 2: select the first <pre> element (of the .runnable-example <div>)
+    var el = findFirst(rouletteChildNode.children, function(e) { return e.nodeName == "PRE";});


Why not getElementsByTagName?

Also, it looks like querySelector is good to use today:
http://caniuse.com/#feat=queryselector

Also, I believe you can avoid traversing the element's children by setting a class on the rouletteChild element.

wilzbach · 2017-06-27T19:46:37Z

Hmm, now that we don't do any layout modifications, I can simply drop the three lines in question - all we do is style.block = "display" after all -> even tinier diff :)

wilzbach · 2017-06-27T20:06:13Z

Btw if we already spent so much time on the examples, I really like how kotlin presents the first example:

https://kotlinlang.org

CyberShadow · 2017-06-27T20:10:48Z

all we do is `style.block = "display" after all -> even tinier diff :)

Awesome. So the coveted 5 lines of JS are possible after all :)

Regarding the buttons: I won't be going for the container hack as imho the proper solution is to do the entire layout at once and not splitting it into pieces, but after this PR anyone can go down this road ;-)

Honestly, I don't think it's any more of a hack that specifying the dimensions of an image on the <img> tag itself. I think it's a good idea in general for any page element that's not trivial to lay out.

wilzbach · 2017-06-27T20:10:50Z

OT: Not so sure whether you noticed, but someone pointed out on the NG had the fair point that the examples lack explanation and explanatory links. Python.org puts a good bar here:

https://www.python.org/

There's also a lot more on this discussion, e.g. how complex the examples should be etc.

CyberShadow · 2017-06-27T20:12:36Z

Yes, I saw that. Eye of the beholder etc. - the impression of any example depends on the reader's experience, and D has a pretty wide target audience. Not sure we can reasonably approach this without an overhaul of the front page.

wilzbach · 2017-06-27T20:13:58Z

CyberShadow added the auto-merge label 2 minutes ago

I think you forgot to approve? Maybe we should add a warning to the bot?
-> dlang/dlang-bot#103

CyberShadow · 2017-06-27T20:14:42Z

Could have sworn I did. Maybe I just forgot to submit it.

wilzbach · 2017-06-27T20:16:14Z

the impression of any example depends on the reader's experience, and D has a pretty wide target audience.
Not sure we can reasonably approach this without an overhaul of the front page.

CC @popen2
IIRC you wanted to create a new, fancy frontpage in the next few weeks? :)

wilzbach · 2017-06-27T20:35:38Z

FYI: the dlang-bot didn't receive the approval hook (they weren't sent). I have enabled this for all dlang repos now, but it needs a bit of handling on the dlang-bot side as well (though I need to wait a bit until a receive a relevant payload to put it into the testsuite).

See also: dlang/dlang-bot#104 or dlang/dlang-bot#108

wilzbach mentioned this pull request Jun 27, 2017

Fix JavaScript example roulette #1757

Closed

CyberShadow reviewed Jun 27, 2017

View reviewed changes

wilzbach force-pushed the fix-js-roulette2 branch from 8e962fe to 51f7f9a Compare June 27, 2017 19:12

CyberShadow reviewed Jun 27, 2017

View reviewed changes

wilzbach mentioned this pull request Jun 27, 2017

Allow to select an example on the front page #1777

Merged

wilzbach force-pushed the fix-js-roulette2 branch from 51f7f9a to 6d8eb93 Compare June 27, 2017 19:45

Fix redraw flashes at the JavaScript example roulette

6d8eb93

CyberShadow added the auto-merge label Jun 27, 2017

CyberShadow approved these changes Jun 27, 2017

View reviewed changes

wilzbach mentioned this pull request Jun 27, 2017

listen for approve events dlang/dlang-bot#104

Closed

wilzbach added auto-merge and removed auto-merge labels Jun 27, 2017

dlang-bot merged commit 842aad8 into dlang:master Jun 27, 2017

wilzbach deleted the fix-js-roulette2 branch June 27, 2017 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix redraw flashes at the JavaScript example roulette #1775

Fix redraw flashes at the JavaScript example roulette #1775

wilzbach commented Jun 27, 2017

dlang-bot commented Jun 27, 2017

CyberShadow Jun 27, 2017

wilzbach Jun 27, 2017

CyberShadow Jun 27, 2017

CyberShadow Jun 27, 2017

wilzbach Jun 27, 2017

CyberShadow Jun 27, 2017

CyberShadow Jun 27, 2017

wilzbach Jun 27, 2017

CyberShadow Jun 27, 2017

wilzbach Jun 27, 2017

CyberShadow Jun 27, 2017

CyberShadow Jun 27, 2017

wilzbach commented Jun 27, 2017

CyberShadow Jun 27, 2017

CyberShadow Jun 27, 2017

CyberShadow Jun 27, 2017

CyberShadow Jun 27, 2017

wilzbach commented Jun 27, 2017 •

edited

Loading

wilzbach commented Jun 27, 2017

CyberShadow commented Jun 27, 2017

wilzbach commented Jun 27, 2017

CyberShadow commented Jun 27, 2017

wilzbach commented Jun 27, 2017

CyberShadow commented Jun 27, 2017

wilzbach commented Jun 27, 2017

wilzbach commented Jun 27, 2017 •

edited

Loading

Fix redraw flashes at the JavaScript example roulette #1775

Fix redraw flashes at the JavaScript example roulette #1775

Conversation

wilzbach commented Jun 27, 2017

dlang-bot commented Jun 27, 2017

Bugzilla references

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

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

wilzbach commented Jun 27, 2017

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

wilzbach commented Jun 27, 2017 • edited Loading

wilzbach commented Jun 27, 2017

CyberShadow commented Jun 27, 2017

wilzbach commented Jun 27, 2017

CyberShadow commented Jun 27, 2017

wilzbach commented Jun 27, 2017

CyberShadow commented Jun 27, 2017

wilzbach commented Jun 27, 2017

wilzbach commented Jun 27, 2017 • edited Loading

wilzbach commented Jun 27, 2017 •

edited

Loading

wilzbach commented Jun 27, 2017 •

edited

Loading