mezerotm / cmv

CMV is a citySDK based map visualizatoin tool
5 stars 5 forks source link

Adding Help to Alpha Release #38

Closed hardnett closed 7 years ago

hardnett commented 7 years ago

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

hardnett commented 7 years ago

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.

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

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)

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

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 commented 7 years ago

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.

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.

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?

hardnett commented 7 years ago

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 commented 7 years ago

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