make site responsive and add feedback link #267

johngruen · 2017-07-20T21:53:29Z

Fixes #203
Fixes #214
Also made the logo clickable for the sake of SEO/providing a link to Test Pilot somewhere on the page!

johngruen · 2017-07-20T21:55:22Z

Note: the Experiment name and Firefox Testpilot are left W/O L10n since they're both proper names

pdehaan · 2017-07-20T23:20:01Z

public/main.css

@@ -262,6 +333,7 @@ tbody {
 #copy {
  display: flex;
  flex-wrap: nowrap;
+  width: 640px;


Should this be a fixed width, or something like max-width?

No, it's fixed...shrinks at the breakpoints at the bottom

pdehaan · 2017-07-20T23:23:21Z

public/main.css

@@ -289,14 +363,16 @@ tbody {
 }

 #copy-btn {
-  width: 165px;
+  padding-left: 10px


public/main.css 366:17 ✖ Missed semicolon CssSyntaxError

pdehaan · 2017-07-20T23:25:12Z

public/resources/feedback.svg

+            </g>
+        </g>
+    </g>
+</svg>


Nit: This is pretty small, but we should probably run it through SVGO or something so it is optimized.

pdehaan · 2017-07-20T23:27:58Z

views/layouts/main.handlebars

-  </div>
+  <header class="header"> 
+    <div class="send-logo">
+      <img src="/resources/send_logo.svg"/>


Not sure if we want an alt attr here, or some data-l10n-id ~~(although I don't suspect this alt="" would need to be translated if it is just "Firefox Send")~~.

Nevermind, I just re-read your #267 (comment) about non-localized. But the alt="" comment still stands.

pdehaan · 2017-07-20T23:30:00Z

views/layouts/main.handlebars

+        <div data-l10n-id="siteSubtitle">web experiment</div>
+      </div>
+    </div>
+    <a href="https://qsurvey.mozilla.com/s3/txp-firefox-send" class="feedback" target="_blank">Feedback</a>


Not sure if we want rel="noreferrer noopener" here. Also, probably want "Feedback" localized.

pdehaan · 2017-07-20T23:32:50Z

views/layouts/main.handlebars

+      <img src="/resources/send_logo.svg"/>
+      <h1 class="site-title">Send</h1>
+      <div class="site-subtitle">
+        <a href="https://testpilot.firefox.com">Firefox Test Pilot</a>


Should this have a target="_blank" and/or would "Firefox Test Pilot" ever be localized? I'm assuming not, per mozilla/testpilot /locales/zh-CN/app.ftl:7-10, but figured I'd verify (considering it's in the TestPilot .ftl files).

Test Pilot should not be localized

pdehaan · 2017-07-20T23:58:22Z

Looks like some UI jiggle on both the label and circular progress bar. Sorry for the lame screen recording, seems like licecap stops recording randomly.

pdehaan · 2017-07-21T00:43:31Z

public/main.css

+  background-size: 14px;
+  border-radius: 3px;
+  border: 1px solid  #0297F8;
+  box-shadow: 0 1px 2px rgba(0,0,0,.1);


I think this will fail our CSS linting rules too. Probably need spaces after commas and leading zeros on the numbers. Sorry. $ npm run format and $ npm run lint may be able to catch/fix some of these.

johngruen · 2017-07-21T09:57:47Z

Wobble should now be resolved

johngruen · 2017-07-21T09:58:21Z

.stylelintrc

@@ -1,6 +1,6 @@
 extends: stylelint-config-standard

 rules:
-  color-hex-case: upper
+  color-hex-case: lower


The npm run format converted all the hex to lower, so i flipped this rule to make everyone's life easier

johngruen · 2017-07-21T10:05:22Z

@dannycoates Just calling out that i ran npm run format before pushing and it changed some JS formatting that I didn't touch by hand.

pdehaan · 2017-07-21T17:08:40Z

@johngruen Looks like this will need a rebasin' on frontend/src/upload.js.

ericawright

around 590px it gets a horizontal scroll bar. I can't find the element that's pushing it though

ericawright · 2017-07-24T13:46:48Z

frontend/src/upload.js

@@ -110,15 +113,21 @@ $(document).ready(function() {
      });
      if (progress[1] < 1000000) {
        $('.progress-text').text(
-          `${file.name} (${(progress[0] / 1000).toFixed(1)}KB of ${(progress[1] / 1000).toFixed(1)}KB)`
+          `${file.name} (${(progress[0] / 1000).toFixed(
+            1


npm run format does this, and it's terrible.

dnarcese · 2017-07-24T14:11:27Z

around 590px it gets a horizontal scroll bar. I can't find the element that's pushing it though

I believe this is because of the popup on the delete button. I can fix this when I restyle it

dnarcese · 2017-07-24T14:26:45Z

public/main.css

+.site-subtitle {
+  font-size: 12px;
+  margin: 0 8px;
+  color: #0c0c0d;


The title and subtitle should be #3E3D40, and the text should be a little smaller compared to the img, according to mocks:

ericawright · 2017-07-24T14:40:12Z

public/main.css

+  }
+}
+
+@media (max-width: 520px) {


This needs a (max-device-width: 520px), rule added, and the one above too. Since pixel ratios on mobile usually don't conform to pixel ratios on computers.
(You can test this by going to the device simulator on Chrome where they take into account the pixel ratio)

I think text sizes need to be larger as well on smaller screens

pdehaan · 2017-07-24T16:29:00Z

public/main.css

+}
+
+.site-title {
+  color: #3E3D40;


public/main.css 41:10 ✖ Expected "#3E3D40" to be "#3e3d40" color-hex-case

pdehaan · 2017-07-24T16:29:19Z

public/main.css

+.site-subtitle {
+  font-size: 12px;
+  margin: 0 8px;
+  color: #3E3D40;


52:10 ✖ Expected "#3E3D40" to be "#3e3d40" color-hex-case

pdehaan · 2017-07-24T16:29:51Z

public/main.css

+
+.site-subtitle a {
+  font-weight: bold;
+  color: #3E3D40;


57:10 ✖ Expected "#3E3D40" to be "#3e3d40" color-hex-case

I'm definitely the only person who gets this treatment

@johngruen Only because I love. ❤️

pdehaan · 2017-07-24T16:30:17Z

public/main.css

+
+@media (max-device-width: 520px) {
+
+  .header {


614:3 ✖ Unexpected empty line before rule rule-empty-line-before

…ive-and-feedback

pdehaan reviewed Jul 20, 2017

View reviewed changes

pdehaan reviewed Jul 21, 2017

View reviewed changes

johngruen force-pushed the responsive-and-feedback branch from d5ff1d1 to 60f0ef7 Compare July 21, 2017 09:57

johngruen commented Jul 21, 2017

View reviewed changes

make site responsive and add feedback link

4633935

johngruen force-pushed the responsive-and-feedback branch from 60f0ef7 to 4633935 Compare July 21, 2017 10:00

ericawright reviewed Jul 24, 2017

View reviewed changes

dnarcese reviewed Jul 24, 2017

View reviewed changes

ericawright reviewed Jul 24, 2017

View reviewed changes

johngruen force-pushed the responsive-and-feedback branch 2 times, most recently from 5f365f8 to a9a3533 Compare July 24, 2017 16:22

pdehaan reviewed Jul 24, 2017

View reviewed changes

Merge branch 'master' of https://github.com/mozilla/send into respons…

dde2c4d

…ive-and-feedback

johngruen force-pushed the responsive-and-feedback branch from a9a3533 to dde2c4d Compare July 24, 2017 16:31

dannycoates merged commit 85068e9 into master Jul 24, 2017

dannycoates deleted the responsive-and-feedback branch July 31, 2017 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make site responsive and add feedback link #267

make site responsive and add feedback link #267

johngruen commented Jul 20, 2017 •

edited

johngruen commented Jul 20, 2017

pdehaan Jul 20, 2017

johngruen Jul 21, 2017

pdehaan Jul 20, 2017

pdehaan Jul 20, 2017

pdehaan Jul 20, 2017 •

edited

pdehaan Jul 20, 2017

pdehaan Jul 20, 2017

johngruen Jul 21, 2017

pdehaan commented Jul 20, 2017

pdehaan Jul 21, 2017

johngruen commented Jul 21, 2017

johngruen Jul 21, 2017

johngruen commented Jul 21, 2017

pdehaan commented Jul 21, 2017

ericawright left a comment

ericawright Jul 24, 2017

dnarcese commented Jul 24, 2017

dnarcese Jul 24, 2017

ericawright Jul 24, 2017

ericawright Jul 24, 2017

pdehaan Jul 24, 2017

pdehaan Jul 24, 2017

pdehaan Jul 24, 2017

johngruen Jul 24, 2017

pdehaan Jul 24, 2017

pdehaan Jul 24, 2017

make site responsive and add feedback link #267

make site responsive and add feedback link #267

Conversation

johngruen commented Jul 20, 2017 • edited

johngruen commented Jul 20, 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

pdehaan Jul 20, 2017 • edited

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

pdehaan commented Jul 20, 2017

Choose a reason for hiding this comment

johngruen commented Jul 21, 2017

Choose a reason for hiding this comment

johngruen commented Jul 21, 2017

pdehaan commented Jul 21, 2017

ericawright left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnarcese commented Jul 24, 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

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

johngruen commented Jul 20, 2017 •

edited

pdehaan Jul 20, 2017 •

edited