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

Adding Help to Alpha Release #38

Merged
merged 3 commits into from
Mar 6, 2017
Merged

Adding Help to Alpha Release #38

merged 3 commits into from
Mar 6, 2017

Conversation

hardnett
Copy link
Collaborator

@hardnett hardnett commented Mar 4, 2017

This is the help overlay to satisfy issue #22.

Here are some examples of what it looks like:
screen shot 2017-03-04 at 9 34 53 am
screen shot 2017-03-04 at 9 35 04 am
screen shot 2017-03-04 at 9 35 18 am

This is the help overlay to satisfy issue #22.
@hardnett hardnett added this to the Alpha milestone Mar 4, 2017
@hardnett hardnett self-assigned this Mar 4, 2017
Copy link
Owner

@mezerotm mezerotm left a comment

Choose a reason for hiding this comment

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

Code Comments

  • We still have display.sidebar.help.js I suggest we either rename intro.js to better fit our naming convention or at least remove display.sidebar.help.js since that code is now deprecated.

  • This is something I had thought of doing too (introducing third-party code) but I feared that it would lead down the road of 'unused code' and improper modularity I don't know how much of this code is bare minimum, but I hope we can look into maybe importing a CDN minified and then writing what we need in our code base. We already have the challenge of loading a lot of geo data and drawing it on a map I would dislike for a help feature to be the additional cause of a 5 second loading time.

  • Here are some webstorm code inspections I ran on the file: js.intro.js-03-04-17.zip
    Here's the tl;dr
    - you're using a lot of var's try using let's
    - you're redeclaring a lot of variables
    - you have many 'spelling mistakes' (this is stuff like combined words it considers "introjs" to be a spelling mistake)

*These are just notes for the sake of documentation

Superficial Comments

Tiny Things I Noticed

  • The skip button is essentially useless since at any moment I can click anywhere on the screen and it will stop the presentation. On that same note, clicking anywhere on the screen skips the presentation, I've already skipped it twice by accident myself now. We need to make it so you can only skip by clicking the skip button.
    (can be fixed at Line:54)

  • For steps 1 & 2 the step indicator is off screen. This is not a big issue to me, but i figured I would document it.
    (can be fixed at Line:56)

  • Step 8 (education icon highlight): Implies there are more variable options after by stating "Press this icon to select a education variable, OR" this can be fixed by removing the OR

Additional 'featured' Notes

  • would be nice if when we get to step 2 (the first time it mentions variables) if we could instead of listing more variables, is instead open the variable up for them or force them to open it and show the steps of clicking a radio button and then going through the process of clicking submit. I imagine a more forceful tutorial where they cannot progress until they comply with the instructions.

  • I think we should stop calling them variables, in terms of when trying to explain it to the public. We can obviously still call them variables but I think variables doesn't sound friendly.

@hardnett
Copy link
Collaborator Author

hardnett commented Mar 5, 2017

Code Comments (Replies)

We still have display.sidebar.help.js I suggest we either rename intro.js to better fit our naming convention or at least remove display.sidebar.help.js since that code is now deprecated.

  • Yes, this code will be removed along with the placeholder code in index.html and a few PNG images. This can be done after this merge since there is another merge of index.html in Location input #37.

This is something I had thought of doing too (introducing third-party code) but I feared that it would lead down the road of 'unused code' and improper modularity I don't know how much of this code is bare minimum, but I hope we can look into maybe importing a CDN minified and then writing what we need in our code base. We already have the challenge of loading a lot of geo data and drawing it on a map I would dislike for a help feature to be the additional cause of a 5 second loading time.

  • I am not sure why this is a concern. The intro.js file is 61KB, and the 2 css files combined are 20KB. Thats 81 KB combined that is loaded once. (the previous solution involved loading 2 PNG images that were 64KB+ combined each time the help-button was clicked). Minification would be a concern if this were a larger library; for example, JQuery unminified is > 200K and minified is hovering around 84K. Our total load time is 47ms and scripting time during loading is about 1s. There are about 20ms for rendering. So our total is barely over a 1s. This added module is probably 1-2 ms. Using 3rd party code like this is pretty standard on web apps: CitySDK, Google Maps, etc. The added time for us to build our own code to do this is an added dev time for non-core requirement. We also have access to the code for updates.

Here are some webstorm code inspections I ran on the file: js.intro.js-03-04-17.zip

  • The webstorm code inspections are useful as a diagnostic, and would be more of an issue for the core of our app. I'd be interested in knowing what it gives us for our code and for the CItySDK code. Logan may be interested in that as well. Because of the minimal impact to our app, this is duly noted, but not a priority in my mind. Others may feel differently.

Superficial Comments

The skip button is essentially useless since at any moment I can click anywhere on the screen and it will stop the presentation. On that same note, clicking anywhere on the screen skips the presentation, I've already skipped it twice by accident myself now. We need to make it so you can only skip by clicking the skip button.
(can be fixed at Line:54)

  • You can also exit the presentation by pressing ESC. Why is having multiple ways to escape a problem? We can hide the skip button as well. I am not sure what "line:54" references. Which file?

For steps 1 & 2 the step indicator is off screen. This is not a big issue to me, but i figured I would document it.
(can be fixed at Line:56)

  • Yes, I noticed it as well. There was no quick fix for this. I suspect it will require changing the CSS they have for building the UI. All of their themes put the number in the upper-lefthand corner, and so changing their code to allow the position of the number to change like the position of the tooltip is a possibility, Or it may be that we pad our screen with a margin at the top of 20 or so pixels to ensure there is space (this may be easier and won't sacrifice much. But its not something for this PR to handle.

Step 8 (education icon highlight): Implies there are more variable options after by stating "Press this icon to select a education variable, OR" this can be fixed by removing the OR

  • Good Catch. No issue there.

Additional 'featured' Notes

These are good points for discussion. Open an issue for each one and tag them as research. They can be for discussion in the Beta Release in my opinion because they do not impact the core functionality of the Alpha Release.

@mezerotm
Copy link
Owner

mezerotm commented Mar 5, 2017

I am not sure why this is a concern. The intro.js file is 61KB, and the 2 css files combined are 20KB. Thats 81 KB combined that is loaded once. (the previous solution involved loading 2 PNG images that were 64KB+ combined each time the help-button was clicked). Minification would be a concern if this were a larger library; for example, JQuery unminified is > 200K and minified is hovering around 84K. Our total load time is 47ms and scripting time during loading is about 1s. There are about 20ms for rendering. So our total is barely over a 1s. This added module is probably 1-2 ms. Using 3rd party code like this is pretty standard on web apps: CitySDK, Google Maps, etc. The added time for us to build our own code to do this is an added dev time for non-core requirement. We also have access to the code for updates.

  • File size is always just a concern to me, maybe it's a bad habit of myn but it's something I enjoy about programming. You do make a valid point my PNG's are worse than what you have introduced as a solution, forgive my hypocrisy.

The webstorm code inspections are useful as a diagnostic, and would be more of an issue for the core of our app. I'd be interested in knowing what it gives us for our code and for the CItySDK code. Logan may be interested in that as well. Because of the minimal impact to our app, this is duly noted, but not a priority in my mind. Others may feel differently.

  • Running these inspections is fairly easy I can run a report on anything you want that would be no problem. Anyone can runs these in fact as long as they have a product from intellij.

You can also exit the presentation by pressing ESC. Why is having multiple ways to escape a problem? We can hide the skip button as well. I am not sure what "line:54" references. Which file?

  • Maybe you understood me wrong. I like the skip button and I have no issue having multiple ways of exiting the presentation I was just pointing out that I accidently excited the presentation to often. I was referring to js/intro.js#Line:54 & js/intro.js#56 these two are just boolean values so I was implying we switch them to false.

@hardnett
Copy link
Collaborator Author

hardnett commented Mar 5, 2017

You can also exit the presentation by pressing ESC. Why is having multiple ways to escape a problem? We can hide the skip button as well. I am not sure what "line:54" references. Which file?

Maybe you understood me wrong. I like the skip button and I have no issue having multiple ways of exiting the presentation I was just pointing out that I accidently excited the presentation to often. I was referring to js/intro.js#Line:54 & js/intro.js#56 these two are just boolean values so I was implying we switch them to false.

  • Okay. I gotcha. In general, I do not like the idea of updating code in libraries because if we update the library and then they have a future release (which they will), then the same issue may still be there. I would suggest making a pull-request to the authors on their github about it and let them decide if or how the code should be fixed or they may propose the intended path for what we want to accomplish. I think the easier solution for us is to use the "hiding" of the skip button. This is probably their intended path for addressing this issue. Frankly, I had not used the skip button at all when I was playing with it, and so it never occurred to me. But let's make an issue for it so its not forgotten, and I'll fix it in another update. Its not hard to hide it.

@hardnett
Copy link
Collaborator Author

hardnett commented Mar 6, 2017

@mezerotm are we good to go? Please approve if I can do this merge.

@hardnett hardnett merged commit 6e47a76 into master Mar 6, 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.

None yet

2 participants