implement "server card" design #2839

trevj · 2016-12-06T00:53:06Z

This implements much of the final "server card" design:

flexbox-based card layout
connect/disconnect graphics
cards expand to fill all available space when there's just one, shortening a little when there are multiple contacts

Things to note:

I removed the text box - we should implement the "empty server list" mock next, and rely on URL interception in the meantime.
I'm still checking with UX w.r.t. typography (standard Material Design, or what's in the mocks).

This change is

trevj · 2016-12-06T15:24:22Z

Apologies, I forgot to attach screenshots.

fortuna · 2016-12-07T00:18:09Z

Are we completely removing the option of explicitly adding a server?
Also, should we make it clear there's a card by adding a margin? It would be nice to have the same UI whether it's one or more items too.

trevj · 2016-12-07T01:01:59Z

Correct. I see nothing in the mocks with which to explicitly input an access code. The mocks show a screen with which to replace the server list when the server list has no entries. That isn't done yet - I don't think that mock was there when I started this PR :-)
Again, I'm just going by the UX mocks.

trevj · 2016-12-07T01:02:13Z

Forgot to reference this issue:
#2815

fortuna · 2016-12-07T00:20:04Z

src/cca/scripts/background.ts

+      return new UproxyServerRepository(
+        getLocalStorage(), core, vpnDevice);
+    } catch (e) {
+      console.warn('local storage unavailable');


Why do you need this mock repository?

So I have something to pass to UproxyServerRepository when testing in Chrome.

fortuna · 2016-12-07T00:20:28Z

src/cca/scripts/background.ts

-    chrome.app.window.create('index_vulcanized.html', null, (appWindow) => {
+    chrome.app.window.create('index_vulcanized.html', {
+      // When running as a Chrome extension for the first time,
+      // open in a nice Nexus 5-like 16:9 aspect ratio (vertically).


Why Nexus 5 ratio?

I figured it's a common aspect ratio. I replaced the explicit reference to a Nexus 5 with a pretty swish list of all common mobile devices.

fortuna · 2016-12-07T18:35:21Z

src/cca/assets/button/connected.svg

@@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>


The connected icon is svg, but the disconnected is png. Should we stick to svg?

Yes, it's weird - but that's what I got with UX. I will follow up with them.

Now 100% SVG!

fortuna · 2016-12-07T19:21:23Z

src/cca/scripts/background.ts

@@ -82,3 +120,19 @@ Promise.all([serversListPagePromise, intents.GetGlobalIntentInterceptor()]).then
    }
  }
 );
+
+// Add some mock servers if we are running as a Chrome extension.
+serversListPagePromise.then((serverListPage) => {


For platform-specific code, I think it's cleaner if we use implementation replacement.
In this case, we could replace the ServerRepository with one containing the two demo servers.

Interesting idea. That would also let me mock out unreachable servers which is a limitation of the current approach. Let me get back to you on this one.

OK, I did this - now if localStorage is unavailable a mock ServerRepository is returned. This allows me easily mock unreachable server behaviour and in doing so I discovered a bug in server_list.ts - thanks :-)

fortuna · 2016-12-07T19:23:17Z

src/cca/scripts/uproxy_server.ts

+      }));
+    } catch (e) {
+      console.warn('could not load servers: ' + e.message);
+      return Promise.resolve([]);


Let's throw and let the client decide how to handle.

fortuna · 2016-12-07T19:34:43Z

src/cca/ui_components/server_card.ts

+
+  private disconnected(msg: string) {
+    this.switchState(State.DISCONNECTED);
+    this.status.innerHTML = msg;


Use this.status.textContent instead.

fortuna · 2016-12-07T19:35:07Z

src/cca/ui_components/server_card.ts

+    }
+
+    this.switchState(State.CONNECTING);
+    this.status.innerHTML = 'Connecting...';


Use this.status.textContent instead.

Nice. Done throughout.

fortuna · 2016-12-07T20:11:14Z

src/cca/ui_components/server_list.ts

-  public pressAddServer(): Promise<ServerEntryComponent> {
-    return this.servers.addServer(this.addTokenText.value).then((server) => {
+  public enterAccessCode(code: string) {
+    return this.servers.addServer(code).then((server) => {
      this.addServer(server);
    });
  }

  private addServer(server: Server) {


Maybe call this addServerCard and change enterAccessCode to addServer?

fortuna · 2016-12-07T20:13:01Z

src/cca/scripts/background.ts

-          resolve(new ServerListPage(appWindow.contentWindow.document.body, servers));
+          const listElem = appWindow.contentWindow.document.body.querySelector('#server-list') as HTMLElement;
+          const serverListPage = new ServerListPage(listElem, servers);
+          appWindow.contentWindow.addEventListener('resize', () => {


When would a resize trigger? Do we really need to listen to this event?

Correct, it will never trigger on a mobile device. I found it useful during developing but would agree it's not strictly necessary and adding it was quite messy. Happy to remove if you'd prefer.

I think we should instead use CSS + class names for the positioning of the elements.
For example, we could have a class for regular cards, and a class for when it's a single card.

Hmm, might be possible. I'll have to experiment a bit with that.

fortuna · 2016-12-07T20:18:51Z

src/cca/index.html

+    }
+  </style>
+
+  <style is="custom-style">


Why is this custom-style?

I'm using it so I can re-use the Material Design colour vars.

I found some more info on this attribute here:
https://www.polymer-project.org/1.0/docs/api/custom-style

Should we move all the style into this?

We can't because those styles only to web components - if we try to hide, say, the scrollbar here then that fails. I don't very much like this limitation but was quite happy to be able to import colours and opacity...

trevj · 2016-12-07T20:26:10Z

Update: I had broken server persistence. It's now basically identical to master except it can handle the exceptions thrown by the new mock storage.

fortuna · 2016-12-08T20:58:05Z

Reviewed 4 of 8 files at r1.
Review status: 4 of 8 files reviewed at latest revision, 8 unresolved discussions.

src/cca/scripts/background.ts, line 86 at r1 (raw file):

Previously, trevj wrote…

I figured it's a common aspect ratio. I replaced the explicit reference to a Nexus 5 with a pretty swish list of all common mobile devices.

I wonder if this is needed at all. If you resize the window, won't it accommodate things automatically?

Comments from Reviewable

fortuna · 2016-12-08T23:19:02Z

Reviewed 1 of 1 files at r2, 1 of 2 files at r5, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

Comments from Reviewable

trevj · 2016-12-09T15:08:31Z

OK, I was able to use only CSS to control the card height. PTAL!

fortuna · 2016-12-09T17:48:32Z

👍

Reviewed 3 of 4 files at r10, 3 of 4 files at r11.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

src/cca/index.html, line 83 at r11 (raw file):

        real Android device will be this short.
      */
      min-height: 300px;

Not for this PR, but we should move away from using px and use some density independent unit instead, such as pt or em. See https://www.tutorialspoint.com/css/css_measurement_units.htm

src/cca/ui_components/server_card.ts, line 80 at r11 (raw file):

    this.switchState(State.DISCONNECTING);
    this.status.textContent = 'Disconnecting...';
    this.button.setAttribute('src', '../../assets/button/disconnected.svg');

I think it's more maintainable if we avoid .. in paths, restricting relative paths only for descendants and absolute paths otherwise.. Do you know if /assets/button/disconnected.svgworks? Or maybe /www//assets/button/disconnected.svg?

Comments from Reviewable

trevj · 2016-12-09T22:16:41Z

I tried using chrome.extension.getURL to avoid relative URLs but it doesn't seem to be available when I load the app in Chrome:
https://developer.chrome.com/extensions/extension#method-getURL

I think we should leave that for a future PR.

fortuna · 2016-12-09T22:25:36Z

Sounds good to me 👍

Trevor Johnston added 5 commits November 30, 2016 12:49

yoyoyo

6bf01e7

getting somewhere now

5a77e92

Merge branch 'master' into trevj-big-button

1afc047

working now

4fca29d

actually add new files, plus graphics

9683020

trevj assigned fortuna Dec 6, 2016

Merge branch 'master' into trevj-big-button

e133003

fortuna suggested changes Dec 7, 2016

View reviewed changes

fix server persistence

1c7e4c8

trevj force-pushed the trevj-big-button branch from d9bb8c8 to 1c7e4c8 Compare December 7, 2016 20:25

Trevor Johnston added 4 commits December 7, 2016 16:37

add link to common device aspect ratios

0760a11

update serverlistpage map comment

6038cdc

server_list finesse

b4facf3

use textContent where possible

2e0967d

Trevor Johnston added 2 commits December 8, 2016 16:12

Merge branch 'master' into trevj-big-button

927b73d

use a mock serverrepository, fix failed connection handler

832e7d1

Trevor Johnston added 5 commits December 8, 2016 18:49

Merge remote-tracking branch 'origin/master' into trevj-big-button

4575dfa

Merge remote-tracking branch 'origin/master' into trevj-big-button

4beebda

let loadServer callers deal with the error

dd6ed98

avoid risk of html injection for server card title

2ce4598

use only CSS for server card height stuff

d3096e2

shadowing considered harmful! fix saving

d1dbcab

SVGs for all buttons

1d0e066

fortuna approved these changes Dec 9, 2016

View reviewed changes

Merge branch 'master' into trevj-big-button

f69753b

Merge branch 'master' into trevj-big-button

67221bd

trevj merged commit 1f480e4 into master Dec 9, 2016

trevj deleted the trevj-big-button branch December 12, 2016 16:30

arvindkrish unassigned fortuna Nov 14, 2017

		@@ -0,0 +1,23 @@
		<?xml version="1.0" encoding="UTF-8" standalone="no"?>

implement "server card" design #2839

implement "server card" design #2839

Conversation

trevj commented Dec 6, 2016 • edited by iislucas Loading

trevj commented Dec 6, 2016

fortuna commented Dec 7, 2016

trevj commented Dec 7, 2016

trevj commented Dec 7, 2016

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

trevj Dec 7, 2016 • edited Loading

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

trevj commented Dec 7, 2016

fortuna commented Dec 8, 2016

fortuna commented Dec 8, 2016

trevj commented Dec 9, 2016

fortuna commented Dec 9, 2016

trevj commented Dec 9, 2016

fortuna commented Dec 9, 2016

trevj commented Dec 6, 2016 •

edited by iislucas

Loading

trevj Dec 7, 2016 •

edited

Loading