Initial my-area layout update #617

elarb · 2018-02-17T14:59:26Z

Used custom icon set project-wide
Added info boxes to lancie-area:
Fixed an issue with gender edit field sometimes being empty
Refactored the profile edit form and slightly changed the layout:

Closes #582

TimvdLippe

Statically looks okay. I think we should start removing all layout usages from the project. Could you remove them from this PR already? Then we can make these incremental changes as well.

TimvdLippe · 2018-02-17T16:05:58Z

src/lancie-my-area/lancie-my-area.html

@@ -15,43 +17,126 @@
      --card-min-width: 300px;
    }

-    .card {
+    .cards-container {
+      @apply --layout-horizontal;


I think we should impose a rule to deprecate all these usages of layout. Both in mixins and in classes. We should use the regular display: flex instead.

TimvdLippe · 2018-02-17T16:08:16Z

src/lancie-my-area/lancie-profile-edit.html

+
+      _fireToast(text) {
+        this.dispatchEvent(new CustomEvent('toast', {
+          detail: {text: text},


Can be just {text}

TimvdLippe · 2018-02-17T16:08:51Z

src/lancie-my-area/my-area-profile.html

+          },
+          pwPlaceholder: {
+            type: String,
+            value: () => { return '&#9679;'.repeat(12);}


martijnjanssen

A lot of comments, but no mayor issues.

martijnjanssen · 2018-02-17T20:26:34Z

src/lancie-my-area/lancie-info-box.html

+      }
+
+      .heading {
+        font-size: 1.2em;


What is the benefit of using em as a measure of size?

Its the same as saying: current font size x1.2

Would it be wise to do this to all font font sizes where it would be logical? Not in this PR, but from now on?

I would say that it slightly improves readability in case of font sizes (vs. using px)

martijnjanssen · 2018-02-17T20:36:08Z

src/lancie-my-area/lancie-info-box.html

+
+      iron-icon {
+        display: block;
+        margin-right: 15px;


Isn't it tidier to only use multiplications of 4px to keep the magic numbers to a minimal?

martijnjanssen · 2018-02-17T20:36:57Z

src/lancie-my-area/lancie-info-box.html

+
+    customElements.define(LancieInfoBox.is, LancieInfoBox);
+  </script>
+


Let's remove this

martijnjanssen · 2018-02-17T20:48:44Z

src/lancie-my-area/lancie-my-area.html

+      --icon-fill-color: var(--paper-grey-800);
+    }
+
+    .discord:hover {


I'm having some difficulty seeing that this is actually clickable. Isn't it a good idea to make it a link or something? Maybe add a slot and put a button in there? I think that the current representation doesn't communicate that it is going somewhere.

True, I experimented with a label showing on hover, but that was pretty ugly :P. Will take a second look at it

A "Join" button or something should make it pretty clear. I guess a <slot> in lancie-info is a good idea for that

martijnjanssen · 2018-02-17T20:49:51Z

src/lancie-my-area/lancie-my-area.html

+      last-response="{{discordWidget}}">
+    </lancie-ajax>
+
+    <lancie-section type="primary full" header="My Area - [[user.profile.displayName]]">


I don't even know why this is a lancie-section, this makes no sense 💩. We can also change this later though.

martijnjanssen · 2018-02-17T21:08:57Z

src/lancie-my-area/my-area-profile.html

        if (request.succeeded) {
-          this.fire('toast', {text: 'Password updated.'});
+          this.dispatchEvent(new CustomEvent('toast', {
+            detail: 'Password updated.',


Let's remove the periods at the end. I'm fairly sure the material docs only recommends adding them with complete sentences.

martijnjanssen · 2018-02-17T21:09:40Z

src/lancie-my-area/my-area-profile.html

        <paper-tooltip for="checkIcon" offset="0" animation-delay="1">Save changes</paper-tooltip>
      </div>
    </paper-card>

  </template>
  <script>
-  'use strict';
-
-  (function() {


Is it not longer needed to encapsulate the js in a function?

Not sure, why was that needed before?

IIRC this was to ensure that no variables were leaking. I think @TimvdLippe can shed some more light on this.

martijnjanssen · 2018-02-17T21:10:26Z

src/lancie-my-area/my-area-seat.html

@@ -65,7 +67,8 @@
          type: Array,
          value: function() {
            return [];
-          }
+          },
+          notify: true,


Same here for the this.set('seats', someVar)

martijnjanssen · 2018-02-17T21:11:18Z

src/lancie-my-area/my-area-teams.html

@@ -119,7 +119,10 @@ <h2>Your personal invites</h2>
  Polymer({
    is: 'my-area-teams',
    properties: {
-      teams: Array,
+      teams: {


I think it's smart to add a default assignment here, too.

martijnjanssen · 2018-02-17T21:19:11Z

src/lancie-ticket-page/lancie-ticket-page.html

@@ -131,7 +131,7 @@

        <div>
          <h2>Fill in your profile</h2>
-          <profile-edit-form id="profileForm" user="{{user}}" on-profile-submitted="profileConfirmed"></profile-edit-form>
+          <lancie-profile-edit id="profileForm" user="{{user}}" on-profile-submitted="profileConfirmed"></lancie-profile-edit>


I'm not sure, but it looks odd to the left side. Centering looks like an improvement over having it on the left side. A margin: 0 auto; on the lancie-profile-edit does the trick.

elarb · 2018-02-17T22:08:15Z

Todo:

Make it clear that the discord info-box is clickable
Don't update when nothing changed (Only send request when the profile is changed #519)
Reset form when profile update fails (Reset form when profile update fails #505)
Make sure Password change validation in my-area not working correctly #321 is fixed

…le, improve mobile responsiveness

TimvdLippe

Looks good! Needs some more testing, but I would rather merge this sooner rather than later. It is quite a lot of changes. Next time, it would be better to split up the various improvements that are included in this 1 PR.

TimvdLippe · 2018-02-24T13:55:23Z

Per our internal discussion: we are holding off this PR until after the LAN. There are no major fixes included in this PR, mostly quality of life improvements. Therefore we rather hold off to not incorporate unnecessary risk so close before the event.

elarb · 2018-02-24T14:43:49Z

@TimvdLippe Agreed, especially since we are pretty close to the event. Also agreeing with splitting up these kind of changes in the feature.

elarb · 2018-03-13T17:53:11Z

Do we continue to work on my-area in this PR, or merge this and continue the rework in other PR's? I'd suggest the latter.

TimvdLippe · 2018-03-13T17:57:58Z

Sure, but we have to address the comments of @martijnjanssen in that PR. Maybe we can split up this PR and do the small steps one at a time? E.g. close this and then create a PR for each commit (or small set of commits)

elarb added 12 commits February 16, 2018 17:56

Only use custom icon set

d1c2cef

Initial my-area layout update

22bcacd

Add base bg color

44ce982

Set icon colors, my-area profile to ES6

38bc0af

Add 'Other' to genders

1bfcf62

Add top and bottom margin

33bf242

Remove unused function

e00e320

Add default string to seat label

f3e0d2e

Refactor profile edit form

0f3f867

Remove unnecessary reflection to attribute

bbb30fc

Change msg-toast color to white

dc0b5e0

Use length to check for array elements

cdccd92

elarb requested a review from martijnjanssen February 17, 2018 15:07

TimvdLippe reviewed Feb 17, 2018

View reviewed changes

Don't apply layout mixins or classes

7c6b8ef

martijnjanssen reviewed Feb 17, 2018

View reviewed changes

martijnjanssen and others added 5 commits February 17, 2018 22:28

Vertically center & layout in an easier way

05dc63e

Margin to 16px

d0d97c7

Remove unnecessary duration

67db15a

Remove dots and use function for firing a toast event

276c694

center lancie-profile-edit in ticket page

aa16fb1

elarb added the work in progress label Feb 17, 2018

elarb added 2 commits February 18, 2018 10:47

Remove lancie-section, make it clear that discord info-box is clickab…

a3e0db0

…le, improve mobile responsiveness

Don't send request when nothing is changed

e66c147

TimvdLippe approved these changes Feb 24, 2018

View reviewed changes

TimvdLippe mentioned this pull request Mar 13, 2018

Add drawer layout only to my-area #483

Closed

elarb closed this Mar 21, 2018

elarb deleted the refactor_elements branch October 19, 2018 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial my-area layout update #617

Initial my-area layout update #617

elarb commented Feb 17, 2018 •

edited

Loading

TimvdLippe left a comment

TimvdLippe Feb 17, 2018

TimvdLippe Feb 17, 2018

TimvdLippe Feb 17, 2018

martijnjanssen left a comment

martijnjanssen Feb 17, 2018

elarb Feb 17, 2018

martijnjanssen Feb 17, 2018

elarb Feb 17, 2018

martijnjanssen Feb 17, 2018

martijnjanssen Feb 17, 2018

martijnjanssen Feb 17, 2018

elarb Feb 17, 2018

skamoen Feb 17, 2018

martijnjanssen Feb 17, 2018

martijnjanssen Feb 17, 2018

martijnjanssen Feb 17, 2018

elarb Feb 17, 2018

martijnjanssen Feb 17, 2018

martijnjanssen Feb 17, 2018

martijnjanssen Feb 17, 2018

martijnjanssen Feb 17, 2018

elarb commented Feb 17, 2018 •

edited

Loading

TimvdLippe left a comment

TimvdLippe commented Feb 24, 2018

elarb commented Feb 24, 2018

elarb commented Mar 13, 2018

TimvdLippe commented Mar 13, 2018


		customElements.define(LancieInfoBox.is, LancieInfoBox);
		</script>

Initial my-area layout update #617

Initial my-area layout update #617

Conversation

elarb commented Feb 17, 2018 • edited Loading

TimvdLippe left a 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

martijnjanssen left a 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

elarb commented Feb 17, 2018 • edited Loading

TimvdLippe left a comment

Choose a reason for hiding this comment

TimvdLippe commented Feb 24, 2018

elarb commented Feb 24, 2018

elarb commented Mar 13, 2018

TimvdLippe commented Mar 13, 2018

elarb commented Feb 17, 2018 •

edited

Loading

elarb commented Feb 17, 2018 •

edited

Loading