Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

Mobile responsive issue fix #128

Merged

Conversation

LuckyGStar
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

@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
Copy link
Owner

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
Copy link
Contributor Author

@rpaschoal
Thanks.

Will make additional commit for option value configuration.

And will trying to add tests as well.

Regards

Andy

@LuckyGStar
Copy link
Contributor Author

LuckyGStar commented Jul 15, 2019

@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
Copy link
Contributor Author

@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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Cheers

Andy

@rpaschoal
Copy link
Owner

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
Copy link
Contributor Author

LuckyGStar commented Jul 22, 2019

@rpaschoal

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

Will add tests soon. 😃

Cheers!

@LuckyGStar
Copy link
Contributor Author

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

Cheers

Andy

@rpaschoal
Copy link
Owner

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!

@rpaschoal rpaschoal changed the base branch from master to 2.0.5 August 23, 2019 21:33
@rpaschoal rpaschoal merged commit d72ce5c into rpaschoal:2.0.5 Sep 1, 2019
@rpaschoal rpaschoal mentioned this pull request Sep 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants