Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Share Functions under single button in MessageBox #4033

Closed
wants to merge 13 commits into from

Conversation

tgxn
Copy link
Contributor

@tgxn tgxn commented Aug 17, 2016

@RocketChat/core
@rodrigok

Closes:
#4038 - Incorrect conditional when rendering the send button.

Took some time to think about how this would best work, and I think this comes close to a implementation that I'm happy with.

Based on PR #4006, I moved the buttons around and added an expanding section for the "Share" buttons.

Still requires browser testing, as I only tested in Chrome and Mobile Chrome/Rocket.Chat App (Android).

LTR
= Hidden
image
= Expanded
image

RTL
= Hidden
image
= Expanded
image

@tgxn tgxn changed the title Better mb Share Functions in single button in MessageBox Aug 17, 2016
@tgxn tgxn changed the title Share Functions in single button in MessageBox Share Functions under single button in MessageBox Aug 17, 2016
@dytec
Copy link

dytec commented Aug 17, 2016

Tested on Chrome and Safari
Chrome: working
Safari: only the attachment button shows up

image

@tgxn
Copy link
Contributor Author

tgxn commented Aug 17, 2016

@dytec Thanks for giving it a go - I'm not able to try it out for myself as it seems that Apple decided to stop producing Safari for Windows. :/

My housemate has a Macbook, so I'll be able to trial it once I get home today.
If you have any suggestions or a fix @dytec please let me know or possibly submit a pull against my branch?

@dytec
Copy link

dytec commented Aug 17, 2016

@tgxn I´ve digged a little bit and it seems that this behavior is not related to your PR.

The following screenshots are from a fresh pull of the develop branch without your PR:

Chrome:
image

Safari:
image

I´ve to dig deeper...

@dytec
Copy link

dytec commented Aug 17, 2016

BTW it seems that in Safari the Geolocation API is available ONLY when connected to WIFI...

@tgxn
Copy link
Contributor Author

tgxn commented Aug 17, 2016

Hmm, I think it might be due to the css allowing too much space for the buttons (when they are hidden, which happens when certain API's aren't available).

@dytec
Copy link

dytec commented Aug 17, 2016

I´m not sure if it´s a CSS issue because the missing buttons are not available in the DOM:

image

@engelgabriel
Copy link
Member

@dytec we had a bug with Safari, did you test the latest version from develop?

Thanks for the PR @tgxn, and we agree the we should have the option to group all attachments into a single button, but we still need to keep the emoji on the oposite side, so we don't lose the alignment with the text on top.

image

@dytec
Copy link

dytec commented Aug 17, 2016

@engelgabriel Yeah fresh download from develop a few minutes ago...

@engelgabriel
Copy link
Member

This is how Slack looks like today

image

@tgxn
Copy link
Contributor Author

tgxn commented Aug 17, 2016

@engelgabriel
I moved the emoij picker back to the left hand side. I agree it looks better this way with the text aligned.
image

…e Attachments, added the option to group the attachment buttons, added i18n strings for new settings, updates existing attachment packages to reflect new settings section
@tgxn
Copy link
Contributor Author

tgxn commented Aug 17, 2016

Updated screenshots:

New Settings Section
image

Grouped-Open
image

Grouped-Closed
image

Ungrouped
image

Now I also tested out some different combination of enabled/disabled options, here's some examples.
image
image

I would also like to as the Rocket.Chat team's opinion on Issue #3991 - Regarding location permission onload rather than onrequest. I think it would be best if (if enabled) the button should show up for everyone, and if the location API is unavailable on the user's device, we can show an error post-click.

This resolves the "Why is this website asking for my location on load" and "How do we know if the user has the right location APIs enabled" by pushing the responsibility back onto the user to have an available location API, and we can also guide users on how to enable/allow location access.

@geekgonecrazy
Copy link
Contributor

@tgxn you rock! Thanks for the contribution! The more attachment types we add the worse this has gotten. This is a fantastic addition! 👍

Will take a look at mentioned issue.

@engelgabriel engelgabriel added this to the 0.50.0 milestone Jan 11, 2017
@engelgabriel engelgabriel modified the milestones: 0.50.0, Short-term Jan 24, 2017
@engelgabriel engelgabriel modified the milestones: 0.55.0, Short-term Apr 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants