New capture code route and view at /v2 #660
Conversation
|
|
|
Sure @jywarren, but I was thinking if it is a good way to directly use the dependency files from the config.assets.paths << Rails.root.join("node_modules")Or should I stick with installing the dependencies to the assets folder ? |
this sounds like a great idea! Sometimes, it's been nice to be able to directly link to a publicly readable file from our node_modules, like a stylesheet or something. So i've found it helpful for that folder to be placed within /public/ -- but see how it works, we can always adjust later! |
|
To speed up GitPod startup, why don't we add to the gitpod config -- i think we need to upgrade the bundler version? spectral-workbench/.gitpod.yml Line 11 in 6e778f5 |
|
Regarding Safari, why don't we open an issue for it for now and keep documenting. It could be related to Flot being so old (or a flot safari isssue? https://github.com/flot/flot/search?q=safari&type=issues), and it's possible the fix is really to upgrade to a newer graphing library. But let's not let it block other progress just yet! Notice the old version also doesn't work in Safari. |
app/assets/javascripts/capture.js
Outdated
| @@ -1,5 +1,15 @@ | |||
| //= require graph.js | |||
| // window.webcam.getCameraList() | |||
There was a problem hiding this comment.
OK, so here, we shouldn't need to change this file at all - in fact, we should be able to point the new html file at the newly upgraded library from #668 -- and perhaps for the time being we should avoid any changes to application.js so as to not touch any existing working systems. By separating out the included JS and CSS files for the new interface, we keep the dependencies for each interface separate until we're sure it's all working. How does that sound?
There was a problem hiding this comment.
I thought maybe the old capture interface could benefit from the new integration since that would also allow for cross browser support on the old interface along with the new one...
| @@ -0,0 +1,363 @@ | |||
| <%= stylesheet_link_tag "new-capture" %> | |||
|
|
|||
There was a problem hiding this comment.
Here we can link directly to the relevant JS files from #668 to work outside of the asset pipeline until we get this page working. Can we try that?
waridrox
force-pushed
the
newCapture
branch
3 times, most recently
from
fe52654
to
c9bfc4b
Compare
|
Hi @jywarren 👋, just added a new route and view for the new capture interface at |
waridrox
changed the title
|
Sure, here's the view at The styling hasn't been added to the page yet 😅. |
|
Ah, ok do create a separate PR for the same and style the page :p |
|
OK, i'm merging now after testing this out. I'd like to think a bit about how we can use system tests to better protect our core functions such as the capture, display, and upload functions. I think we have some test coverage but not sure how comprehensive it is. Also, I noted that GitPod is having some trouble as our Gemfile.lock specifies an older version of bundler and so we need to install it with |
|
|
|
Great work @waridrox!!!! |
|
Let's at least ensure that the new capture page is built with good system test protection. If it's possible to write some basic system tests for other existing pages as well it may speed our work by reducing uncertainty whenever we merge! |
Part of #645
Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!
rake testNot sure if it is a great idea to create JS and CSS files in the assets folder...
So, this would probably require a lot of re-factoring but the integration works on both the old and the new url at /v2 on Chrome and Firefox. Safari is still out of luck :( Need some help there
CC: @jywarren @pydevsg