rpaschoal / ng-chat

💬 A simple facebook/linkedin lookalike chat module for Angular applications.
MIT License
155 stars 92 forks source link

Mobile responsive issue fix #128

Closed LuckyGStar closed 5 years ago

LuckyGStar commented 5 years ago

Ng-Chat isn't displayed in mobile devices.

As long as, most of the users are using mobile devices, we should display them in mobile devices.

:1: Chat components are hidden in mobile devices. So fixed it that it is displayed in mobile devices. :2: Changed components sizes so that they have same width. :3: They can be switched using collapse functionality.

Regards

Andy Lee

rpaschoal commented 5 years ago

Hi @LuckyGStar ,

Thanks for raising this PR with these changes. Before I pull your branch and perform some manual testing on it, I believe we should add a setting instead of commenting the viewport calculation.

The viewport calculation might be helpful for some people if they only need ng-chat on a web view and it is pretty handy to not expand the total viewport when the width of chat windows exceed it. Something like viewportWidthCalculationEnabled ?

What do you think? Cheers

LuckyGStar commented 5 years ago

@rpaschoal Thank you for your quick response.

Yes, I think it would be better to make a new option value for specific users.

We can make new option variable like viewportOnMobileEnabled?.

If new option is enabled, we can make components to be enabled in mobile devices.

What do you think?

Regards

Andy

rpaschoal commented 5 years ago

Sweet Andy,

Let's call it mobileViewportEnabled for brevity and let's keep it disabled by default. We can later on update the documentation and add the description of what this setting does.

Looking forward to seeing the new commit with the setting in place. Would be nice to have some unit tests in place for this behavior too. I could help you out with this if you need a hand, let me know.

Cheers!

LuckyGStar commented 5 years ago

@rpaschoal
Thanks.

Will make additional commit for option value configuration.

And will trying to add tests as well.

Regards

Andy

LuckyGStar commented 5 years ago

@rpaschoal I have made option variable name as isViewportOnMobileEnabled to meet javascript coding style guide.

I am writing tests now and will make new commit soon.

Regards

Andy

LuckyGStar commented 5 years ago

@rpaschoal I have made additional commit with updated code for media query and option variable definition.

FYI, I have uploaded screenshot of mobile cases. Please give me your opinion for my changes.

Screenshot at Jul 19 13-05-04

Cheers

Andy Lee

rpaschoal commented 5 years ago

I wonder if we should update the Friends list to have the same width as the chat window when the media query points to a mobile view?

LuckyGStar commented 5 years ago

I wonder if we should update the Friends list to have the same width as the chat window when the media query points to a mobile view?

I was just tried to highlight friends list with other chat components. If you think so, yes, we can make them the same width.

Okay?

Regards

Andy

LuckyGStar commented 5 years ago

@rpaschoal I have made friends list the same width with chat components.

Cheers

Andy

rpaschoal commented 5 years ago

Hi @LuckyGStar ,

Thank you. I'll see if I find a spare time to review these and introduce it with a new release. It might take a couple days as I'm a bit busy 😅

Any luck with the unit tests?

Cheers!

LuckyGStar commented 5 years ago

@rpaschoal

Sorry for missing tests. Luckily I am not busy because don't have active projects yet. 😢

Will add tests soon. 😃

Cheers!

LuckyGStar commented 5 years ago

@rpaschoal Hope you are doing well. I have added default value test for isMobileEnabled option. Detailed tests required as well?

Cheers

Andy

rpaschoal commented 5 years ago

Thank you @LuckyGStar ,

Always good to unit test the added behavior as well if possible.

I haven't had the time yet to test these changes locally, still trying to find some spare time... Thinking of releasing some extra things with these changes too!

Cheers!