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

Revert "Revert "Update pegasus templates to use markdown versions of HTML strings"" #33930

Merged
merged 3 commits into from Apr 1, 2020

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Mar 30, 2020

Reverts #33925, restoring #33567

Depends on the problem string having been fixed on staging: 194f961#diff-575ae35f1160e39fc3c9b240757fbf95R18-R23

Confirmed with #32977 that the issue has been resolved:

Before

diff --git a/pegasus_render_cache/csedweek.org b/pegasus_render_cache/csedweek.org
index d739087..ead850a 100644
--- a/pegasus_render_cache/csedweek.org
+++ b/pegasus_render_cache/csedweek.org
@@ -185,7 +185,9 @@ j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
 <div class="container_responsive">
 <div>
 <div class="lines_of_code_header" style="background-color: #d43f3a">
-<a href="/csteacher" style="color: white; text-decoration: none; font-weight: 400">Are you a CS teacher? See other CSEdWeek resources</a>
+<a href="/csteacher" style="color: white; text-decoration: none; font-weight: 400">
+Are you a CS teacher? See other CSEdWeek resources
+</a>
 </div>
 <br>
 <style>
@@ -402,9 +404,8 @@ j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
 <div id="hoc2014">
 <div class="count" style="font-size: 42px; line-height: 42px; font-family: 'Gotham 5r', sans-serif;">
 <a href="/leaderboards" style="text-decoration:none">
-<h1>Try an</h1>
-<h2>Hour of Code</h2>
-<h3>426,370,561 served</h3>
+<h1>Try an\n\n## Hour of Code\n\n### 426,370,561 served</h1>
+
 </a>
 <h4 class="desktop-feature">Anybody can learn.</h4>
 </div>

After

diff --git a/pegasus_render_cache/csedweek.org b/pegasus_render_cache/csedweek.org
index d739087..5801595 100644
--- a/pegasus_render_cache/csedweek.org
+++ b/pegasus_render_cache/csedweek.org
@@ -185,7 +185,9 @@ j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
 <div class="container_responsive">
 <div>
 <div class="lines_of_code_header" style="background-color: #d43f3a">
-<a href="/csteacher" style="color: white; text-decoration: none; font-weight: 400">Are you a CS teacher? See other CSEdWeek resources</a>
+<a href="/csteacher" style="color: white; text-decoration: none; font-weight: 400">
+Are you a CS teacher? See other CSEdWeek resources
+</a>
 </div>
 <br>
 <style>
@@ -403,8 +405,11 @@ j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
 <div class="count" style="font-size: 42px; line-height: 42px; font-family: 'Gotham 5r', sans-serif;">
 <a href="/leaderboards" style="text-decoration:none">
 <h1>Try an</h1>
+
 <h2>Hour of Code</h2>
+
 <h3>426,370,561 served</h3>
+
 </a>
 <h4 class="desktop-feature">Anybody can learn.</h4>
 </div>

@Hamms Hamms requested review from wjordan and removed request for wjordan March 30, 2020 22:58
@Hamms
Copy link
Contributor Author

Hamms commented Mar 30, 2020

Looks like there are a number of changes to other templates, too; most of them are minor formatting differences, but still worth updating.

@Hamms Hamms force-pushed the revert-33925-revert-33567-markdownify-pegasus branch from 32ef68c to 9eb0380 Compare March 30, 2020 23:38
@Hamms
Copy link
Contributor Author

Hamms commented Mar 30, 2020

So, there are actually a bunch of places in pegasus where we are incorrectly including text directly in divs without first wrapping them in paragraphs like HTML says we're supposed to. In my original implementation, I updated several of these to be wrapped in paragraphs, and I thought I had visually verified that there was no difference as a result, but I now realize I was wrong and doing so actually added a bunch of padding in unexpected places.

So for now, I'm updating the markdown to preserve existing (incorrect) behavior, and we can investigate fixing these individually and with attention to the visuals.

You can see a full diff of all HTML changes resulting from this PR here

@Hamms Hamms requested review from breville and wjordan March 30, 2020 23:41
@Hamms Hamms merged commit 9cd54e4 into staging Apr 1, 2020
@Hamms Hamms deleted the revert-33925-revert-33567-markdownify-pegasus branch April 1, 2020 20:18
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.

None yet

2 participants