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

added modal to check if chrome browser #531

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

Conversation

leesan11
Copy link
Contributor

@leesan11 leesan11 commented Jun 6, 2018

Hi @stevekrouse , this is my initial crack at the issue. I created a modal that will inform the client if they are not using chrome. Unfortunately, I don't know if the placing of the function declaration and function call is in a good spot in the code. Please provide me some feedback! Thank you!

@stevekrouse
Copy link
Owner

[This is an automated integration to preview this pull request's changes to the website.]

https://cdn.rawgit.com/leesan11/WoofJS/e104fb2b6cc71fc5d5c67c4a7ca897dec5d6c20b/index.html

Copy link
Owner

@stevekrouse stevekrouse left a comment

Choose a reason for hiding this comment

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

Great start! Again, apologies for the nit-picking feedback.

One extra thing: this pop-up may become super annoying to users of different browsers. One quick solution is to drop a little cookie in their browser that says that they already say this popup so we don't need to show it to them again. (We could store this information in the user in the database, but I think in the browser makes more sense for this case.) Could you add that as well?

create.html Outdated
@@ -1690,6 +1702,16 @@ <h4 class="modal-body text-danger" :style="{display: !getID() ? 'block' : 'none'
// helper function to prevent runCode from running too often
var debouncedRunCode = debounce(runCode, 1000, false)

function checkChrome() {

Copy link
Owner

Choose a reason for hiding this comment

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

remove empty line here

create.html Outdated
if (!(str.indexOf("Edge") == -1 && str.indexOf("OPR") == -1 && str.indexOf("Firefox") == -1 && str.indexOf("Trident") == -1)) {
$("#chromeModal").modal("show");
}

Copy link
Owner

Choose a reason for hiding this comment

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

remove empty line

create.html Outdated
@@ -1690,6 +1702,16 @@ <h4 class="modal-body text-danger" :style="{display: !getID() ? 'block' : 'none'
// helper function to prevent runCode from running too often
var debouncedRunCode = debounce(runCode, 1000, false)

function checkChrome() {

var str = window.navigator.userAgent;
Copy link
Owner

Choose a reason for hiding this comment

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

I'd probably make this a const declaration and change the variable name to userAgent

create.html Outdated
function checkChrome() {

var str = window.navigator.userAgent;
if (!(str.indexOf("Edge") == -1 && str.indexOf("OPR") == -1 && str.indexOf("Firefox") == -1 && str.indexOf("Trident") == -1)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Usually when you have a very specific expression like this that you got from the internet (I'm guessing), you want to include the URL where you got it from for context as a comment in your code.

From my 5 min of research, this seems like the most reliable answer.

create.html Outdated

var str = window.navigator.userAgent;
if (!(str.indexOf("Edge") == -1 && str.indexOf("OPR") == -1 && str.indexOf("Firefox") == -1 && str.indexOf("Trident") == -1)) {
$("#chromeModal").modal("show");
Copy link
Owner

Choose a reason for hiding this comment

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

It doesn't really matter, but I'd consider making checkChrome() return a boolean, and then show the model if it's true. If you wanted to do that, you could rename the method to isChromeBrowser so that it reads better as if(!isChromeBrowser()) { $("#chromeModal").modal("show"); }

create.html Outdated
<button type="button" class="close" data-dismiss="modal" aria-label="Close"><span aria-hidden="true">&times;</span></button>
<h4 class="modal-title"><span class="glyphicon glyphicon-share-alt"></span> Chrome Browser Not Detected!</h4>
</div>
<h4 class="modal-body text-danger" :style="{display: !getID() ? 'block' : 'none'}">The Chrome Browser is recommended. You may experience technical difficulties with other browsers.</h4>
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this here? {display: !getID() ? 'block' : 'none'}

Copy link
Owner

Choose a reason for hiding this comment

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

You could add a link to download chrome: https://www.google.com/chrome/

Copy link
Owner

Choose a reason for hiding this comment

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

I'd re-word this: "WoofJS.com works best with the Chrome web browser. We're a small team, so we can't test new features on all browsers. If you are having difficulties with WoofJS.com, consider downloading Chrome here."

create.html Outdated
<div class="modal-content">
<div class="modal-header">
<button type="button" class="close" data-dismiss="modal" aria-label="Close"><span aria-hidden="true">&times;</span></button>
<h4 class="modal-title"><span class="glyphicon glyphicon-share-alt"></span> Chrome Browser Not Detected!</h4>
Copy link
Owner

Choose a reason for hiding this comment

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

Remove the share glyhicon <span class="glyphicon glyphicon-share-alt"></span>. It's ok to copy and paste code, but do be sure do remove the code that's no longer relevant

Copy link
Owner

Choose a reason for hiding this comment

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

I'd probably rename the title to "WoofJS is Optimized for Chrome"

@leesan11
Copy link
Contributor Author

leesan11 commented Jun 8, 2018

@stevekrouse these are the changes I made with the new commit. I am unsure if the way I added the cookie is the proper way to do it, however, it works. I didn't copy and paste code (userAgent), however, I got the ideas from different sources (e.g. look for the word Trident), do I still need to reference them in the code? Please give me more feedback to fix my commits. Thank you!

Tasks Completed:

  • removed glyhicon
  • renamed to WoofJS
  • removed {display: !getID..}
  • added download link
  • reworded message
  • removed empty lines
  • changed var to const
  • changed method name to isChromeBrowser (returns boolean now)
  • added cookie feature (not sure if done properly)

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.

2 participants