Several UI fixes #136

DRSDavidSoft · 2017-08-01T23:53:48Z

According to the individual commits, minor fixes made to several parts of the UI.

1. Removed duplicated rule 2. Fixed scroll issue

Looks better than `overflow: hidden`.

This looks better to the eye, IMHO.

1. Background is set to `cover`, this will resize the background to fit the `div` while avoiding aspect ratio issues. 2. Added more space for the logout button.

1. Added more space for it 2. Changed the font size to match the UI overall font-size

The required extra space is achieved using the columns sass module.

1. The vertical padding matches the rest of sub elements' vertical padding 2. The horizontal padding now has enough space for the increment/decrement buttons to look better

This will differentiate the card from the rest of the post, making it easier to follow.

This will make the `x` icon go in the middle of close button

Fixed overflow issues

This will make the app login on hitting enter upon entering authentication information, instead of manually clicking on the login button.

1. Width 2. Button

Required for some positionings to work correctly

This will reflect the changes to the column sass module.

hex2f · 2017-08-02T12:41:45Z

@DRSDavidSoft are you using eslint to check linting before push?

tahnik · 2017-08-02T12:47:56Z

@DRSDavidSoft
Please check the log of Appvoyer or Travis.

Here is the error:
C:\projects\devrantron\app\src\main\js\components\auth\login.js
47:14 error Property should be placed on a new line react/jsx-first-prop-new-line

By the way, welcome to the project and thank you!

Property should be placed on a new line

DRSDavidSoft · 2017-08-02T13:17:54Z

@tahnik Great! Why using two CI services may I ask?

Trailing spaces are not allowed, apparently.

tahnik · 2017-08-02T13:26:22Z

@DRSDavidSoft Travis doesn't support Windows. We use AppVeyor for Windows and Travis for macOS, Linux

DRSDavidSoft · 2017-08-02T13:42:33Z

@tahnik Can you review the PR?

tahnik · 2017-08-02T13:43:15Z

@DRSDavidSoft I did, check above

tahnik · 2017-08-02T14:16:50Z

@DRSDavidSoft have you found the reviews? Just making sure!

hex2f · 2017-08-02T18:28:14Z

app/src/main/res/css/modules/compact_user_card.sass

@@ -51,8 +52,8 @@
      cursor: pointer
  .logout
    position: absolute
-    bottom: 5px
-    left: 10px
+    bottom: 10px


change to 0.25rem

Done. Initially I used px instead of rem because the previous commit also used it.
I suppose most of the units should be using rem here, right?

hex2f · 2017-08-02T18:28:38Z

app/src/main/res/css/modules/compact_user_card.sass

-    bottom: 5px
-    left: 10px
+    bottom: 10px
+    left: 1.5rem


change to 0.5rem instead. Looks a bit odd when it's so high up.

Are you referring to the left property? I set it to 0.5rem, but could you comment on the suitable values for both left and bottom properties?

This will prevent unwanted selection of the text in increment, decrement, score and comments icon.

tahnik · 2017-08-03T13:02:40Z

@DRSDavidSoft could you give me your email so I could add you in slack? It's easier to talk over there. Thanks

tahnik · 2017-08-03T18:48:08Z

@DRSDavidSoft I sent you the request. Please join.

DRSDavidSoft · 2017-08-03T20:36:21Z

Thank you, @tahnik. I joined. What about this PR?

DRSDavidSoft · 2017-08-06T10:41:43Z

The PR is conflicting with the edits. Could you review & merge it before more conflicts to show up?
Also what does the keydown event handler do?

tahnik · 2017-08-06T13:20:45Z

I resolved the conflicts. Can you please look at the reviews here: https://github.com/tahnik/devRantron/pull/136/files? I won't be able to merge this before you fix these stuff.

DRSDavidSoft · 2017-08-06T13:49:17Z

@tahnik If you've reviewed/commented on my commits, I don't seem to find anything from you.
http://imgur.com/a/11CrO

tahnik · 2017-08-06T13:50:43Z

yeah, because I just started to push to your branch and fix those reviews @DRSDavidSoft

tahnik · 2017-08-02T12:57:34Z

app/src/main/js/components/auth/login.js

@@ -44,7 +44,12 @@ class Login extends Component {
        <div className="auth_image" >
          <img alt="devrant" src="./res/images/devrant_sidebar.png" />
        </div>
-        <div className="auth_form" >
+        <div className="auth_form"


We really need this, but this doesn't work for some reason. Can you check?

Edit: The reason is this is a div, not a form. So onSubmit won't be triggered.

Why not change it to a form? It makes sense
Otherwise, we should write a handler on both fields to check e.which == 13 on keypress, which I don't recommend

We have recently fixed it using keyDownEvent. so don't worry about this one.

tahnik · 2017-08-02T13:00:19Z

app/src/main/res/css/modules/bottom_bar.sass

@@ -2,7 +2,7 @@
  font-size: 1.5rem
  display: flex
  background-color: #40415A
-  padding: 0.5rem
+  padding: 0.8rem 1rem


@Dacexi can you review this one?

@RekkyRek I'd be grateful if you did!

tahnik · 2017-08-02T13:02:49Z

app/src/main/res/css/modules/column.sass

-    padding: 0rem 0rem 1rem 0rem
-    height: calc(100vh - 2.8rem - 1.5rem)
+    padding: 0 10px
+    height: 100vh


Don't make it 100vh please. Keep some padding in the bottom. This can cause side effects.

@tahnik Tested with some other situations, I agree.

tahnik · 2017-08-02T13:03:29Z

app/src/main/res/css/modules/column.sass

-    overflow-y: scroll
-    overflow-x: hidden
+    position: relative
+    overflow: auto


Sometimes some rants cause overflow-x issues. It would be better to keep it the way it is.

Nope, it's not. I think I know what you're referring to. Instead of overflow-x, this should be done:
https://stackoverflow.com/q/3058866/1454514

or better - https://stackoverflow.com/a/18706058/1454514

TL;DR: word-wrap: break-word; & word-break: break-all;

You can add word-break: break-all; but it is still better to keep overflow-x hidden.

@tahnik I hear you, but it causes problems with box-shadow. If it's going to be hidden, there should be enough padding for the box-shadow IMHO.

@DRSDavidSoft break-all causes some other problems. For example, it also breaks words which look horrible. box-shadow looks fine now so have a look.

tahnik · 2017-08-02T13:05:05Z

app/src/main/res/css/modules/column.sass

    transform: translateZ(0)
    -webkit-transform: translateZ(0)
-    padding: 0rem 0rem 1rem 0rem
-    height: calc(100vh - 2.8rem - 1.5rem)
+    padding: 0 10px


This is causing the column not to align with the topbar. Is this what you intended? I don't think that looks nice. Maybe @Dacexi can confirm it.

@tahnik Can you post a screenshot from before and after please?

I fixed it in another commit.

tahnik · 2017-08-02T13:07:06Z

app/src/main/res/css/modules/compact_user_card.sass

@@ -51,8 +52,8 @@
      cursor: pointer
  .logout
    position: absolute
-    bottom: 5px
-    left: 10px
+    bottom: 10px


This is causing the shadow in the back to misalign

tahnik · 2017-08-02T13:08:14Z

app/src/main/res/css/modules/item_card.sass

@@ -41,7 +40,7 @@
    width: 100%
 .item_card.shadow
  &:hover
-    box-shadow: 0 10px 20px rgba(0,0,0,0.19), 0 6px 6px rgba(0,0,0,0.23)
+    box-shadow: 0 2px 0px rgba(0, 0, 0, 0.19), 0 3px 4px rgba(0, 0, 0, 0.23)


Don't think it's looking nice either. Looks more like a black border than a shadow.

@tahnik, OK, let's keep the previous one, but please make sure it has enough padding.

tahnik · 2017-08-03T21:16:45Z

app/src/main/js/components/auth/login.js

@@ -44,7 +44,13 @@ class Login extends Component {
        <div className="auth_image" >
          <img alt="devrant" src="./res/images/devrant_sidebar.png" />
        </div>
-        <div className="auth_form" >
+        <div


Please remove this as this is not working :)

Can .auth_form be an actual <form>? :)

tahnik · 2017-08-06T13:52:26Z

fuck me sorry. I forgot to request the changes

tahnik · 2017-08-06T13:52:56Z

can you see the reviews now @DRSDavidSoft

…too close to the columns and messing up the structure

tahnik · 2017-08-06T14:13:47Z

@DRSDavidSoft does everything looks good to you now?

DRSDavidSoft · 2017-08-06T14:23:23Z

@tahnik Seems good! I was wondering why I didn't get any reviews from you. Let's merge it :)

DRSDavidSoft added 14 commits August 2, 2017 03:14

Fixed overflow issues

887ad14

1. Removed duplicated rule 2. Fixed scroll issue

Added a fade effect instead of immediate cut

37b24b2

Looks better than `overflow: hidden`.

Modified excessive use of box-shadow in item cards

d02f88a

This looks better to the eye, IMHO.

Modified user_compact

0f93b74

1. Background is set to `cover`, this will resize the background to fit the `div` while avoiding aspect ratio issues. 2. Added more space for the logout button.

Tuned log out button's position

8f51bb2

1. Added more space for it 2. Changed the font size to match the UI overall font-size

Removed unnecessary margin

ba6abc7

The required extra space is achieved using the columns sass module.

Tuned spacing to meet the description requirements

21f01fa

1. The vertical padding matches the rest of sub elements' vertical padding 2. The horizontal padding now has enough space for the increment/decrement buttons to look better

Added box-shadow to user-card

2ced05c

This will differentiate the card from the rest of the post, making it easier to follow.

Fixes the close icon x

c7e72dd

This will make the `x` icon go in the middle of close button

Merge pull request #1 from DRSDavidSoft/patch-1

cd0f64d

Fixed overflow issues

Updated login.js

f178ffc

This will make the app login on hitting enter upon entering authentication information, instead of manually clicking on the login button.

Tuned user-card spacing

491319a

1. Width 2. Button

Added a relative position to item_container

cb678de

Required for some positionings to work correctly

Added padding

69e51c1

This will reflect the changes to the column sass module.

tahnik self-assigned this Aug 2, 2017

tahnik added the bug A software bug label Aug 2, 2017

Fixed react/jsx-first-prop-new-line issue

d6cda23

Property should be placed on a new line

Fixed no-trailing-spaces

c4b06a8

Trailing spaces are not allowed, apparently.

hex2f reviewed Aug 2, 2017

View reviewed changes

DRSDavidSoft added 3 commits August 3, 2017 15:44

Changed pixel unit to rem

028e14b

Changed left property according to review request

e1004f5

Added user-select: none to bottom container

5bac778

This will prevent unwanted selection of the text in increment, decrement, score and comments icon.

Merge branch 'master' into master

c23a557

tahnik added 2 commits August 6, 2017 14:28

Restoring column styles. Moving from rem to px is not a good idea

02fe1e8

Made the items_container bigger as @DRSDavidSoft suggested

91420c9

tahnik suggested changes Aug 6, 2017

View reviewed changes

tahnik added 3 commits August 6, 2017 14:59

Not sure why you're doing this. This is causing the scroll bar to be …

cf8a773

…too close to the columns and messing up the structure

Box shadow has been verified by other tester. Let's not change it

08e477f

Updated package version

b73ca8c

tahnik approved these changes Aug 6, 2017

View reviewed changes

tahnik merged commit d124f29 into tahnik:master Aug 6, 2017

Several UI fixes #136

Several UI fixes #136

Conversation

DRSDavidSoft commented Aug 1, 2017

hex2f commented Aug 2, 2017

tahnik commented Aug 2, 2017

DRSDavidSoft commented Aug 2, 2017

tahnik commented Aug 2, 2017

DRSDavidSoft commented Aug 2, 2017

tahnik commented Aug 2, 2017

tahnik commented Aug 2, 2017

Choose a reason for hiding this comment

DRSDavidSoft Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DRSDavidSoft Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

tahnik commented Aug 3, 2017

tahnik commented Aug 3, 2017

DRSDavidSoft commented Aug 3, 2017

DRSDavidSoft commented Aug 6, 2017

tahnik commented Aug 6, 2017

DRSDavidSoft commented Aug 6, 2017

tahnik commented Aug 6, 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

DRSDavidSoft Aug 6, 2017 • 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

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

DRSDavidSoft Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

tahnik commented Aug 6, 2017

tahnik commented Aug 6, 2017

tahnik commented Aug 6, 2017

DRSDavidSoft commented Aug 6, 2017

DRSDavidSoft Aug 3, 2017 •

edited

Loading

DRSDavidSoft Aug 3, 2017 •

edited

Loading

DRSDavidSoft Aug 6, 2017 •

edited

Loading

DRSDavidSoft Aug 6, 2017 •

edited

Loading