Improved mobile layout for smaller devices #76

yurm04 · 2016-12-21T18:19:17Z

This pull request addresses Issue #31 and includes:

Additional media queries for better support of smaller mobile devices.
Rearranged message name, time, text order in individual messages.

Small Device

Medium Device

- Rearranged message name, time, text order in individual messages.

codecov-io · 2016-12-21T18:22:25Z

Current coverage is 100% (diff: 100%)

Merging #76 into master will not change coverage

@@           master   #76   diff @@
===================================
  Files           3     3          
  Lines          52    52          
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
  Hits           52    52          
  Misses          0     0          
  Partials        0     0

Powered by Codecov. Last update da1ba39...9d0f2e7

nelsonic · 2016-12-22T08:29:47Z

lib/client.js

    html += '<small class=\'time\'>' + getTime(msg.t) + ' </small>';
-    html += '<span class=\'name\'>' + msg.n + ': </span>';
-    html += '<span class=\'msg\'>' + msg.m + '</span>';
+    html += '<p class=\'msg\'>' + msg.m + '</p>';


I actually like having the date/time first on the line (before) the name because it's a fixed width whereas the name won't be ... so the UI ends up looking more aligned ... @yurm04 what were your thoughts on this?
is this the UI other chat apps are implementing?
(genuine question ... I'm in a "Gitter" bubble so I don't know ...)

@nelsonic swapping the order of the time and name was mostly to mirror other chat apps i've used (Slack, HipChat). I was also toying with the idea of right aligning the timestamp on tablet and desktop (would have required a float if I left the name and timestamp as they were), but decided against it until I got some feedback 😅

I can change the order back before the PR gets merged if that's the preferred layout.

nelsonic · 2016-12-22T08:33:21Z

style.css

  width: 100%;
+  display: flex;


display: flex; is "Flexbox", right...?
https://css-tricks.com/snippets/css/a-guide-to-flexbox/
(I'm a complete noob at CSS ... so zero XP on this...)
It appears to be available in "All Modern Browsers" ... http://caniuse.com/#feat=flexbox
Given that this app uses WebSockets which is also not "progressive enhancement" http://caniuse.com/#search=websockets
I don't have an issue with using Flexbox for this demo app.
@iteles thoughts?

Correct, I changed the message input form to flexbox because it was the quickest and easiest way to fix the mobile layout. However, flexbox doesn't play nice with older IE browsers (below IE 11), so if a more universal solution is necessary I'd be happy to make that change.

While on the subject, are there any thoughts on which browsers should be supported by this project?

My thoughts here is that what we're aiming to teach/help with in this project is the combination of hapi/socket.io/redis that brought you to it in the first place @yurm04!

Whilst we want to make all of our code and examples as accessible as possible, I think that it's not the main aim of this repo, so it shouldn't be a blocker for merging these excellent changes in. There are lots of improvements to the CSS that could be done if we were being picky (e.g. over-usage of ID selectors and descendent selectors)!

Suggested next steps (happy for this to be in a different PR):

Add a line to the readme explaining that we're using flexbox for simplicity given the focus of this repo is on the hapi/socket.io/redis combination, but raising a warning this will not play well with anything under IE11 (sadly still under heavy use)

Add an issue to the backlog for the more universal solution you've suggested @yurm04 so that it's captured as something to be dealt with when you or anyone else has a little time 😊

nelsonic · 2016-12-22T08:40:02Z

style.css

    #messages li:nth-child(odd) { background: #eee; }


 /* == Individual messages == */
-.time { color: #2c3e50; }
+.time { color: #9c9c9c; }


This color is good (more subtle): http://www.colorhexa.com/9c9c9c

the old one was a bit "heavy": http://www.colorhexa.com/2c3e50

nelsonic

@yurm04 thank you so much for digging out this issue and fixing it!! 😮 😍
I'm happy with this code. (LGTM) 👍
The only thing I would change would be to leave the time at the start of the line. But I'm happy to merge just so we can #fix the <button>! 👍

@iteles over-to-you. ❤️ ✅

P.S. @yurm04 how did you discover @dwyl ...? (thanks!)

iteles · 2016-12-22T09:26:29Z

Will review at lunch 👍 Thanks for the PR @yurm04 , would also be great to know how you found dwyl! 😊

yurm04 · 2016-12-22T15:59:04Z

@iteles @nelsonic thank you both 😄 I found the project while searching for a Hapi.js/socket.io implementation as research for a similar project I'm working on. I'm new to Hapi, so this was perfect to get me started in the right direction. The inclusion of Redis was a plus since I was kicking around different ideas for storage, but now I'm sold on this particular setup.

Would love to keep contributing to the project so if there are any other issues/requests I can help with, please shoot them my way!

iteles · 2016-12-23T11:19:23Z

style.css

  width: 100%;
+  display: flex;


My thoughts here is that what we're aiming to teach/help with in this project is the combination of hapi/socket.io/redis that brought you to it in the first place @yurm04!

Whilst we want to make all of our code and examples as accessible as possible, I think that it's not the main aim of this repo, so it shouldn't be a blocker for merging these excellent changes in. There are lots of improvements to the CSS that could be done if we were being picky (e.g. over-usage of ID selectors and descendent selectors)!

Suggested next steps (happy for this to be in a different PR):

Add a line to the readme explaining that we're using flexbox for simplicity given the focus of this repo is on the hapi/socket.io/redis combination, but raising a warning this will not play well with anything under IE11 (sadly still under heavy use)

Add an issue to the backlog for the more universal solution you've suggested @yurm04 so that it's captured as something to be dealt with when you or anyone else has a little time 😊

iteles · 2016-12-23T11:22:13Z

style.css

+  margin-right: 0.8em;
+}
+
+.name,


Hooray for the fact that you're using classes and not ID selectors! 🎉 ❤️
https://github.com/dwyl/style-guide#general-points

iteles · 2016-12-23T11:23:44Z

style.css

+
+/*==============================
+  MEDIA QUERIES
+================================*/


Great to have a separate section for these in this simple case 👍

iteles · 2016-12-23T11:25:04Z

@yurm04 This is fantastic, thank you! ❤️
Super impressed with the organisation and quality of your code, clear commit and PR messages.

I've only suggested one action in the comments above which is not a blocker for merging this in 🎉

Welcome to dwyl!

- Added media queries for better mobile styling.

247a807

- Rearranged message name, time, text order in individual messages.

yurm04 mentioned this pull request Dec 21, 2016

CSS | 'Send' button disappears with smaller width windows #31

Closed

nelsonic added enhancement in-review labels Dec 22, 2016

nelsonic assigned iteles Dec 22, 2016

nelsonic requested review from nelsonic and iteles December 22, 2016 08:27

nelsonic reviewed Dec 22, 2016

View reviewed changes

nelsonic approved these changes Dec 22, 2016

View reviewed changes

switched timestamp and name back

9d0f2e7

iteles approved these changes Dec 23, 2016

View reviewed changes

iteles merged commit ec7377d into dwyl:master Dec 23, 2016

iteles mentioned this pull request Dec 30, 2016

Add to readme on why we've opted for flexbox #78

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved mobile layout for smaller devices #76

Improved mobile layout for smaller devices #76

yurm04 commented Dec 21, 2016 •

edited

codecov-io commented Dec 21, 2016 •

edited

nelsonic Dec 22, 2016

yurm04 Dec 22, 2016

nelsonic Dec 22, 2016

yurm04 Dec 22, 2016

iteles Dec 23, 2016

nelsonic Dec 22, 2016

nelsonic left a comment

iteles commented Dec 22, 2016

yurm04 commented Dec 22, 2016

iteles Dec 23, 2016

iteles Dec 23, 2016

iteles Dec 23, 2016

iteles commented Dec 23, 2016 •

edited

Improved mobile layout for smaller devices #76

Improved mobile layout for smaller devices #76

Conversation

yurm04 commented Dec 21, 2016 • edited

codecov-io commented Dec 21, 2016 • edited

Current coverage is 100% (diff: 100%)

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

nelsonic left a comment

Choose a reason for hiding this comment

iteles commented Dec 22, 2016

yurm04 commented Dec 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iteles commented Dec 23, 2016 • edited

yurm04 commented Dec 21, 2016 •

edited

codecov-io commented Dec 21, 2016 •

edited

iteles commented Dec 23, 2016 •

edited