<table><tr><td style="">broulik accepted this revision.<br />broulik added a reviewer: broulik.<br />broulik added a comment.<br />This revision is now accepted and ready to land.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D2760" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>Lgtm. Some nitpicks below.</p>
<p>For some reason the right border is smaller than the other ones (has been the case without this patch already), so maybe QML Rectangle border leaks outside the window or so :/</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D2760#inline-10809" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">OutputIdentifier.qml:37</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="bright"> </span><span style="color: #aa4000"><span class="bright">anchors.centerIn:</span></span><span class="bright"> </span><span style="color: #004012"><span class="bright">parent</span></span><span class="bright"></span><span class="p"><span class="bright">;</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="bright"> </span><span style="color: #aa4000"><span class="bright">s</span>pacing<span class="bright">:</span></span><span class="bright"> </span><span style="color: #601200"><span class="bright">10</span></span><span class="bright"></span><span class="p"><span class="bright">;</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">width:</span></span><span class="bright"> </span><span style="color: #004012"><span class="bright">childrenRect</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span style="color: #004012"><span class="bright">width</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">+</span></span><span class="bright"> </span><span style="color: #004012"><span class="bright">units</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span style="color: #004012"><span class="bright">largeSpacing</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"> </span><span style="color: #601200"><span class="bright">4</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">height:</span></span><span class="bright"> </span><span style="color: #004012"><span class="bright">childrenRect</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span style="color: #004012"><span class="bright">height</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">+</span></span><span class="bright"> </span><span style="color: #004012"><span class="bright">units</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span style="color: #004012"><span class="bright">largeS</span>pacing<span class="bright"></span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"> </span><span style="color: #601200"><span class="bright">2</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">width: childrenRect.width + 2 * childrenRect.x to reduce magic numbers</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D2760#inline-10811" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">OutputIdentifier.qml:46</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #aa4000">y:</span> <span style="color: #004012">units</span><span class="p">.</span><span style="color: #004012">largeSpacing</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #aa4000">font.pointSize:</span> <span style="color: #004012">theme</span><span class="p">.</span><span style="color: #004012">defaultFont</span><span class="p">.</span><span style="color: #004012">pointSize</span> <span style="color: #aa2211">*</span> <span style="color: #601200">3</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #004012">anchors</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Why not just a Label since you're not using the one thing that makes Heading special (the font size with "level" magic)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D2760#inline-10810" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">OutputIdentifier.qml:47-49</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #004012">anchors</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #aa4000">topMargin:</span> <span style="color: #004012">units</span><span class="p">.</span><span style="color: #004012">largeSpacing</span> <span style="color: #aa2211">*</span> <span style="color: #601200">2</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Remove, you're not setting a top anchor and you set Y already</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>rKSCREEN KScreen</div></div></div><br /><div><strong>BRANCH</strong><div><div>sebas/screenid</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D2760" rel="noreferrer">https://phabricator.kde.org/D2760</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>sebas, Plasma, broulik<br /><strong>Cc: </strong>broulik, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas<br /></div>