Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29354742: Flare HTML example review (Closed)

Created:
Sept. 22, 2016, 3:26 p.m. by juliandoucette
Modified:
Sept. 24, 2016, 3:55 p.m.
Reviewers:
Lisa
Visibility:
Public.

Description

Flare HTML example review

Patch Set 1 #

Patch Set 2 : Added a second example #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+270 lines, -0 lines) Patch
A adding-filter-list.html View 1 1 chunk +139 lines, -0 lines 5 comments Download
A changing-display-options.html View 1 1 chunk +131 lines, -0 lines 3 comments Download

Messages

Total messages: 2
juliandoucette
Sept. 22, 2016, 3:26 p.m. (2016-09-22 15:26:21 UTC) #1
juliandoucette
Sept. 22, 2016, 4:08 p.m. (2016-09-22 16:08:46 UTC) #2
This is not "that bad". 

1. We need better (containing every possible element that commonly appears on a
page) examples

2. We must support HTML5 properly (EG: <strong>, <em>, <aside> in these
examples) or automate the replacement process (EG: <span class="Bold"> ->
<strong>) 

3. We must require @lisa to extract the primary content from the page manually
or provide a tool to automate this process

https://codereview.adblockplus.org/29354742/diff/29354745/adding-filter-list....
File adding-filter-list.html (right):

https://codereview.adblockplus.org/29354742/diff/29354745/adding-filter-list....
adding-filter-list.html:117: <div class="row outer-row"
data-mc-content-body="True">
Everything above this line is unnecessary (not a problem).

https://codereview.adblockplus.org/29354742/diff/29354745/adding-filter-list....
adding-filter-list.html:122: <div class="Note" data-mc-autonum="&lt;span
style=&quot;font-family: 'Source Sans Pro';&quot;
class=&quot;mcFormatFamily&quot;&gt;&lt;span style=&quot;color: #000000;&quot;
class=&quot;mcFormatColor&quot;&gt;NOTE&lt;/span&gt;&lt;/span&gt;"><span
class="autonumber"><span><span style="font-family: 'Source Sans Pro';"
class="mcFormatFamily"><span style="color: #000000;"
class="mcFormatColor">NOTE</span></span></span></span>
This creates the alert box and "NOTE" heading.

- On one hand, it's easy to identify and replace
- On the other, it's hard to read and extract important bits (like the "NOTE"
heading)

https://codereview.adblockplus.org/29354742/diff/29354745/adding-filter-list....
adding-filter-list.html:127: <li value="2">Tap<img
src="../../Graphics/Icon-AndroidMenu.png" class="Callout35px" />and select <span
class="Bold">Settings</span>.</li>
<span class="Bold"> should be <strong>.

https://codereview.adblockplus.org/29354742/diff/29354745/adding-filter-list....
adding-filter-list.html:130: <li value="5">Select the <span class="Bold">filter
list</span> you would like to subscribe to below <span class="Italic">Other
Languages</span>.</li>
<span class"Italic"> should be <em>.

https://codereview.adblockplus.org/29354742/diff/29354745/adding-filter-list....
adding-filter-list.html:133: </section><a class="exit-off-canvas"></a>
Everything below this line is unnecessary (not a problem).

https://codereview.adblockplus.org/29354742/diff/29354745/changing-display-op...
File changing-display-options.html (right):

https://codereview.adblockplus.org/29354742/diff/29354745/changing-display-op...
changing-display-options.html:117: <div class="row outer-row"
data-mc-content-body="True">
Everything above this line is unnecessary (not a problem).

https://codereview.adblockplus.org/29354742/diff/29354745/changing-display-op...
changing-display-options.html:122: <li value="3">Tap <span
class="Bold">Display</span>.<p>The following options are available below <span
class="Italic">Display</span>:</p><ul><li value="1"><span class="Bold">Text size
- </span>By default, the text size is set to medium. To change it, tap <span
class="Bold">Text size</span>. On the <span class="Italic">Text</span> screen,
tap either the small 'A' or the big 'A' to adjust the text size. Tap <span
class="Bold">Set</span>.</li><li value="2"><span class="Bold">Title bar</span> -
 This option allows you to display either the page title or the page address. By
default, page address is selected. To display the page title, tap <span
class="Bold">Title bar</span> and select <span class="Bold">Show page
title</span>.</li><li value="3"><span class="Bold">Full-screen browsing</span> -
If you wish to view the Adblock Browser title bar while scrolling down a page,
tap the checkbox.</li><li value="4"><span class="Bold">Allow autoplay</span> -
Enabled by default, this option controls the autoplay of videos and other media
content. If you do not wish for videos and other media content to play
automatically, tap the checkbox.</li></ul><p>The following options are available
below <span class="Italic">Advanced</span>:</p><ul><li value="1"><span
class="Bold">Character encoding</span> - This option controls the display of the
character encoding menu. By default, the character encoding menu is not visible.
If you would like to view the menu, tap <span class="Bold">Character
encoding</span> and select <span class="Bold">Show menu</span>.</li><li
value="2"><span class="Bold">Plugins</span> - This option allows you to control
plugin preferences.</li></ul></li>
- <span class="Bold"> should be <strong>
- <span class="Italic"> should be <em>
- This is really hard to read
  - We may be able to run a beautifier to automatically format this HTML

https://codereview.adblockplus.org/29354742/diff/29354745/changing-display-op...
changing-display-options.html:125: </section><a class="exit-off-canvas"></a>
Everything below this line is unnecessary (not a problem).

Powered by Google App Engine
This is Rietveld