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

Show download button only for the OS of the client-device. #254

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

agent515
Copy link

@agent515 agent515 commented Feb 3, 2021

Fix #236
Using platform.js to detect OS at the client-side, only display the corresponding button and Github-repo-link.

@vercel
Copy link

vercel bot commented Feb 3, 2021

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/ritz078/moose/9sp24l5uu
✅ Preview: https://moose-git-fork-agent515-download-link.ritz078.now.sh

@ritz078
Copy link
Owner

ritz078 commented Feb 3, 2021

It detects my mac to be a linux.

@agent515
Copy link
Author

agent515 commented Feb 3, 2021

In my machine, it is showing windows and Linux button (instead of github) on vercel only.
local looks fine.

image

@ritz078
Copy link
Owner

ritz078 commented Feb 3, 2021

platform.js might be buggy

@agent515
Copy link
Author

agent515 commented Feb 3, 2021

platform.js might be buggy

Ohh, should I use different package or try out different versions? I tried react-device-detect, it wasn't working properly either. Do you know a reliable package?

@ritz078
Copy link
Owner

ritz078 commented Feb 4, 2021

Why not use platform.os instead of matching string?

Also, if it doesn't match any platform, we should show all the options.

@agent515
Copy link
Author

agent515 commented Feb 6, 2021

Why not use platform.os instead of matching string?

It is an object. So, converted into a string to use regex.

Also, if it doesn't match any platform, we should show all the options.

image

When I comment out GitHub-link, Mac link looks fine. I am not able to understand what's causing this problem.

image

I have made some changes still couldn't resolve that issue. Making some pushes to see if anything is improved.

Use React component to show Download Buttons
Comment on lines 123 to 141
{flag && (
<DownloadButton url={macUrl} logo={mdiApple} disabled={false} />
)}
{flag && (
<DownloadButton logo={mdiMicrosoftWindows} disabled={true} />
)}
{flag && (
<DownloadButton
url={linuxUrl}
logo={mdiLinux}
disabled={false}
/>
)}
{
<DownloadButton
url="https://github.com/ritz078/moose"
logo={mdiGithub}
disabled={false}
/>
Copy link
Owner

Choose a reason for hiding this comment

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

{
  flag && (
    <>
      <DownloadButton url={macUrl} logo={mdiApple} disabled={false} />
      <DownloadButton logo={mdiMicrosoftWindows} disabled={true} />
      <DownloadButton url={linuxUrl} logo={mdiLinux} disabled={false} />
      <DownloadButton
        url="https://github.com/ritz078/moose"
        logo={mdiGithub}
        disabled={false}
      />
    </>
  );
}

@ritz078
Copy link
Owner

ritz078 commented Feb 6, 2021

It is an object. So, converted into a string to use regex.

In that case you can use platform.os.family ?

When I comment out GitHub-link, Mac link looks fine. I am not able to understand what's causing this problem.

Can you try adding a unique key prop to all the DownloadButton components ?

See https://kentcdodds.com/blog/understanding-reacts-key-prop

Also I am thinking that we should have other links too in case someone wants to download a build of other platform.

Screenshot 2021-02-07 at 12 58 53 AM

In the above image "Download for other platforms" is a button. If you click on it, all the links show up. WDYT ?

@agent515
Copy link
Author

agent515 commented Feb 7, 2021

In that case you can use platform.os.family ?

Yeah, this way seems more appropriate.

Can you try adding a unique key prop to all the DownloadButton components ?

I'll try this out.

Also I am thinking that we should have other links too in case someone wants to download a build of other platform.

Screenshot 2021-02-07 at 12 58 53 AM

In the above image "Download for other platforms" is a button. If you click on it, all the links show up. WDYT ?

I agree. This is a better UI/UX.

I'll make those changes and will let you know.

@ritz078 ritz078 closed this Feb 12, 2021
@ritz078
Copy link
Owner

ritz078 commented Feb 12, 2021

closed by mistake.

@ritz078 ritz078 reopened this Feb 12, 2021
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.

Detect OS and provide download link
2 participants