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

Only show install extension item in sidebar if user has matching browser #460

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

GabrielMajeri
Copy link
Contributor

What does it fix?

Partially addresses #367 (in order to also display an icon as suggested, we'd need to first modify InstrumentsItem)

The install extension item is only shown for the current browser.

How has it been tested?

Locally tested it with Chromium and Firefox.

@vercel
Copy link

vercel bot commented Jun 27, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/code4romania/stam-acasa/bwwfrnp3b
✅ Preview: https://stam-acasa-git-fork-gabrielmajeri-fix-install-browse-5027c4.code4romania.vercel.app


const SidebarLayout = ({ children }) => {
// Browser detection based on this answer:
// https://stackoverflow.com/a/9851769/5723188
const isChrome =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: this doesn't seem to work on Chrome mobile.
Screenshot_20200628-003042
But that should not matter as Chrome mobile doesn't support extensions yet.

The Firefox detection works on mobile, which matter the most

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the mechanism for detecting Chrome relies on detecting Chrome extension support, so if mobile doesn't have it, it won't show up.

@surdu
Copy link
Member

surdu commented Jun 27, 2020

I remember seeing a comment from, I want to say @catileptic , where she argued that we want to advertise that we have the extension on any browser, so that the user knows that if he/she uses other web browser than those two, or Internet Explorer, she/he will know about our extensions.

... or something like that

@Utwo
Copy link
Member

Utwo commented Jun 30, 2020

I remember seeing a comment from, I want to say @catileptic , where she argued that we want to advertise that we have the extension on any browser, so that the user knows that if he/she uses other web browser than those two, or Internet Explorer, she/he will know about our extensions.

... or something like that

Yes there was a valid comment on that direction.

@GabrielMajeri
Copy link
Contributor Author

It's more probable that if some user visits the site in a browser, it would be the primary browser they use.

And if the extension seems useful to them, they could always search for the version for their other browser.

@surdu
Copy link
Member

surdu commented Jul 1, 2020

Not if their primary browser, or the browser they have to use at work, is a browser that is not Chrome / Firefox.

There might be valid cases where they use Opera, Safari (most Mac users do) or IE, and they see a value installing/using Chrome selectively just to browse the news and have the benefit of our extensions.

A good compromise would be to show that we have extensions for Chrome and Firefox, and have a button "Install in your browser" if we detect the user is visiting from a supported browser

@GabrielMajeri
Copy link
Contributor Author

@surdu Ok, I've tried out this alternative. This is how it would look.
image
Would this be an appropiate solution?

@surdu
Copy link
Member

surdu commented Jul 1, 2020

How will this look on Safari ?

I was thinking more like having a generic box mentioning that we have extensions for Chrome / Firefox with links to both of them and the Install button appearing only on supported browsers.

@overeha , @catileptic , @RaduCStefanescu any suggestions ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants