devtools-panel.html - Issue 29740563: Noissue - In the devtools panel, preserve space for missing document domain

Side by Side Diff: devtools-panel.html

Issue 29740563: Noissue - In the devtools panel, preserve space for missing document domain (Closed)

Patch Set: Created April 2, 2018, 6:47 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <!--	2 <!--

3 - This file is part of Adblock Plus <https://adblockplus.org/>,	3 - This file is part of Adblock Plus <https://adblockplus.org/>,

4 - Copyright (C) 2006-present eyeo GmbH	4 - Copyright (C) 2006-present eyeo GmbH

5 -	5 -

6 - Adblock Plus is free software: you can redistribute it and/or modify	6 - Adblock Plus is free software: you can redistribute it and/or modify

7 - it under the terms of the GNU General Public License version 3 as	7 - it under the terms of the GNU General Public License version 3 as

8 - published by the Free Software Foundation.	8 - published by the Free Software Foundation.

9 -	9 -

10 - Adblock Plus is distributed in the hope that it will be useful,	10 - Adblock Plus is distributed in the hope that it will be useful,

(...skipping 70 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
81 </div>	81 </div>

82 <footer>	82 <footer>

83 <a id="reload">Reload</a> page to see effect of filter changes	83 <a id="reload">Reload</a> page to see effect of filter changes

84 </footer>	84 </footer>

85	85

86 <template>	86 <template>

87 <tr>	87 <tr>

88 <td>	88 <td>

89 <div class="request-wrapper">	89 <div class="request-wrapper">

90 <a class="resource-link" href="" target="_blank"> </a>	90 <a class="resource-link" href="" target="_blank"> </a>

91 <div class="domain"></div>	91 <div class="domain"> </div>
	Thomas Greiner 2018/04/03 12:39:43 This issue is purely cosmetical so, instead of not This issue is purely cosmetical so, instead of not allowing this element to be empty, why not make sure that neither of the rows here collapses if it's empty? That way we won't have to worry about what's inside those rows. For instance, using CSS this could be achieved with `min-height: 1em`. Sebastian Noack 2018/04/03 23:21:56 Well, this is how we already avoid other elements Show quoted text On 2018/04/03 12:39:43, Thomas Greiner wrote: > This issue is purely cosmetical so, instead of not allowing this element to be > empty, why not make sure that neither of the rows here collapses if it's empty? > That way we won't have to worry about what's inside those rows. Well, this is how we already avoid other elements with optional content (see "resource-link", "filter" and "origin") from collapsing. I didn't intend to do a major refactoring here, but merely made the behavior consistent. Show quoted text > For instance, using CSS this could be achieved with `min-height: 1em`. While I agree that inserting dummy data (i.e. &nbsp) to influence the visual representation is a hack (and against semantic markup practices), I couldn't come up with a better way back then when I implemented the devtools panel. (Redundantly) hard-coding the (min-)height is hardly any less of a hack. Thomas Greiner 2018/04/04 12:34:01 Sure, we should avoid any major refactoring. So if Show quoted text On 2018/04/03 23:21:56, Sebastian Noack wrote: > Well, this is how we already avoid other elements with optional content (see > "resource-link", "filter" and "origin") from collapsing. I didn't intend to do a > major refactoring here, but merely made the behavior consistent. Sure, we should avoid any major refactoring. So if you think the suggested change is out of scope for fixing this issue and if you want to make it consistent, feel free to make two separate commits. Show quoted text > While I agree that inserting dummy data (i.e. &nbsp) to influence the visual > representation is a hack (and against semantic markup practices), I couldn't > come up with a better way back then when I implemented the devtools panel. > (Redundantly) hard-coding the (min-)height is hardly any less of a hack. It's great that we're revisiting code to iterate on it. In this case I don't see how defining how elements should look like in CSS is a hack. For instance, I'd also consider it reasonable to set the anchor tag above to `display: block` to ensure that it occupies the entire width of its parent. So the former defines the link's height and the latter defines its width. Sebastian Noack 2018/04/05 00:36:10 It's a hack to superfluously define a height on th Show quoted text On 2018/04/04 12:34:01, Thomas Greiner wrote: > In this case I don't see how defining how elements should look like in CSS > is a hack. It's a hack to superfluously define a height on the element. This seems much more error-prone than the approach with the  . Currently, the element's height is defined by the font-size/line-height of its content. If we now explicitly define a height for the element itself and later change the size of the content, we have to remember updating the height of the container as well, otherwise the layout is off.
92 </div>	92 </div>

93 </td>	93 </td>

94 <td>	94 <td>

95 <div class="type"></div>	95 <div class="type"></div>

96 </td>	96 </td>

97 <td>	97 <td>

98 <div class="action-wrapper">	98 <div class="action-wrapper">

99 <div class="filter-wrapper">	99 <div class="filter-wrapper">

100 <div class="filter"> </div>	100 <div class="filter"> </div>

101 <div class="origin"> </div>	101 <div class="origin"> </div>

102 </div>	102 </div>

103 </div>	103 </div>

104 </td>	104 </td>

105 </tr>	105 </tr>

106 </template>	106 </template>

107 </body>	107 </body>

108 </html>	108 </html>

OLD	NEW

« no previous file with comments | « background.js ('k') | devtools-panel.js » ('j') | no next file with comments »