Desktop support #309

dparikh · 2018-03-20T17:44:39Z

To submit this PR only once associated CL is approved.

dvoytenko · 2018-03-20T19:11:48Z

src/components/dialog.css

@@ -24,12 +24,52 @@
 * The maximum width allowed for the dialog is 480 px. Default width
 * is 100% (for container width < 480 px).
 */
-@media (min-width: 480px) {
+@media (max-width: 640px), (max-height: 640px) {


Update the description above to reflect the new rules.

dvoytenko · 2018-03-20T19:12:28Z

src/components/dialog.css

@@ -24,12 +24,52 @@
 * The maximum width allowed for the dialog is 480 px. Default width
 * is 100% (for container width < 480 px).
 */
-@media (min-width: 480px) {
+@media (max-width: 640px), (max-height: 640px) {
  .swg-dialog,
  .swg-toast {
    width: 480px !important;


Hmm. What if the viewport is less than 480px wide? If covered by the other media rules, please add a comment here. E.g. this property will be further restricted by X rule.

dvoytenko · 2018-03-20T19:15:10Z

src/components/dialog.css

  .swg-dialog,
  .swg-toast {
    width: 480px !important;
    left: -240px !important;
    margin-left: calc(100vw - 100vw / 2) !important;
+    box-sizing: border-box;


It'd seem the box-sizing should be the same for all variants?

dvoytenko · 2018-03-20T19:16:13Z

src/components/dialog.css

+    left: 0 !important;
+    right: 0 !important;
+    margin-left: 0 !important;
+    box-sizing: border-box;


dvoytenko · 2018-03-20T19:16:55Z

src/components/dialog.css

+}
+
+/**
+ * Desktop/Large screen support. When device with or height is > 640px.


"with" -> "width"

it's "AND" right? Not "or"?

Done & Done.

dvoytenko · 2018-03-20T19:20:07Z

src/ui/ui.css

+ * Since the desktop view has the parent iframe (.swg-dialog) transperant,
+ * this style adds the background with borders to the loading indicator.
+ */
+@media (min-width: 630px) {


No min-height expression here?

dvoytenko · 2018-03-20T19:21:02Z

src/ui/ui.css

+  }
+}
+
+.swg-loading-container {


Why are media rules above the main declaration?

dvoytenko · 2018-03-20T19:21:53Z

src/ui/ui.css

+}
+
+.swg-loading-container {
+  position: fixed !important;


Hmm. position:fixed seems wrong here. And also in the swg-loading below. I think these all should be position:absolute.

dvoytenko · 2018-03-20T19:22:27Z

src/ui/ui.css

+
+.swg-loading-container {
+  position: fixed !important;
+  width: 550px !important;


Most of properties appear to be the same as above?

dvoytenko · 2018-03-20T19:23:28Z

src/ui/ui.css

+@media (min-width: 630px) {
+  .swg-loading-container {
+    width: 550px !important;
+    height: 148px !important;


Hmm. What happens in the transition states? E.g. when offers view transits to the pay confirmation? It'd seem like this will be broken with loading container laid out over/under size and double shadow. No?

I have tested this few times, and transition seems working fine.

dvoytenko · 2018-04-09T18:41:19Z

src/ui/loading-view.js

@@ -32,8 +32,13 @@ export class LoadingView {
    /** @private @const {!Document} */
    this.doc_ = doc;

+    this.loadingContainer_ = createElement(this.doc_, 'div', {
+      'class': 'swg-loading-container',


Use swg-loading-container as the tag name per previous change. Also make sure you immediately add swg-loading-container {display: block}.

dvoytenko · 2018-04-09T18:41:38Z

src/ui/ui.css

@@ -19,18 +19,44 @@ body {
  margin: 0;
 }

+.swg-loading-container {


switch to tag

dvoytenko · 2018-04-09T18:41:46Z

src/ui/ui.css

+ * this style adds the background with borders to the loading indicator.
+ */
+@media (min-width: 630px), (min-height: 630px) {
+  .swg-loading-container {


dvoytenko · 2018-04-09T18:42:09Z

src/ui/ui.css

 swg-container,
 swg-loading,
 swg-loading-animate,
 swg-loading-image {
  display: block;
 }

+swg-loading-container,


Hmm. Here's it's already tag. Pls ensure this is tested.

This was a duplicate. Tests are already in place.

dparikh requested a review from dvoytenko March 20, 2018 17:44

dvoytenko suggested changes Mar 20, 2018

View reviewed changes

dparikh added 7 commits April 9, 2018 09:54

Desktop support

cb3d457

fix unit tests

54b8ec1

fix unit tests

ae64ff2

Loading indicator unit tests fixes

d297299

review fixes

fc80e06

wip

5281ca5

review fixes

216b7e9

dparikh force-pushed the desktop-1 branch from af8b0fe to 216b7e9 Compare April 9, 2018 16:57

dparikh added 2 commits April 9, 2018 10:13

Make loading dialog centered

a2ba9a0

Fix unit test for loading-view

07a7602

dvoytenko suggested changes Apr 9, 2018

View reviewed changes

review fixes, convert class to tag

ebcff20

dvoytenko approved these changes Apr 9, 2018

View reviewed changes

Added a comment to clarify the iframes for swg-dialog, swg-toast

1321d6e

dparikh merged commit afde55e into subscriptions-project:master Apr 9, 2018

dparikh deleted the desktop-1 branch April 9, 2018 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop support #309

Desktop support #309

dparikh commented Mar 20, 2018

dvoytenko Mar 20, 2018

dparikh Mar 20, 2018

dvoytenko Mar 20, 2018

dparikh Mar 20, 2018

dvoytenko Mar 20, 2018

dparikh Mar 20, 2018

dvoytenko Mar 20, 2018

dparikh Mar 20, 2018

dvoytenko Mar 20, 2018

dparikh Mar 20, 2018

dvoytenko Mar 20, 2018

dparikh Apr 9, 2018

dvoytenko Mar 20, 2018

dparikh Apr 9, 2018

dvoytenko Mar 20, 2018

dparikh Apr 9, 2018

dvoytenko Mar 20, 2018

dparikh Apr 9, 2018

dvoytenko Mar 20, 2018

dparikh Apr 9, 2018

dvoytenko Apr 9, 2018

dparikh Apr 9, 2018

dvoytenko Apr 9, 2018

dparikh Apr 9, 2018

dvoytenko Apr 9, 2018

dparikh Apr 9, 2018

dvoytenko Apr 9, 2018

dparikh Apr 9, 2018

Desktop support #309

Desktop support #309

Conversation

dparikh commented Mar 20, 2018

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

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

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

Choose a reason for hiding this comment