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

feat(view): Add viewer rewrite to monorepo #104

Merged
merged 10 commits into from
Mar 6, 2019
Merged

feat(view): Add viewer rewrite to monorepo #104

merged 10 commits into from
Mar 6, 2019

Conversation

mcous
Copy link
Member

@mcous mcous commented Feb 21, 2019

overview

This PR is part 3 of mcous#1, and adds the new viewer app to the monorepo. It's written in Typescript and still needs tests, but this (I think) is feature-complete enough to launch.

Preview at: https://staging.tracespace.io/view

Still TODO in follow up PR(s):

  • Contributing documentation updates
  • Set up CI deploy
  • Set up Greenkeeper
  • Set up offline support
  • Component consolidation and simplification
  • Way more tests

changelog

Improvements from the code living at tracespace/viewer:

try it out

# install dependencies
cd tracespace
npm i

# launch dev server on localhost:8080
# also launches pcb-stackup, pcb-stackup-core, gerber-to-svg dev servers
npm start

# build production assets and serve them on localhost:9090
npm run build
npm run serve

@kasbah
Copy link
Collaborator

kasbah commented Feb 21, 2019

So cool! It's looking great, as always really impressed by the code quality and organization too (makes my projects look like a pile of random source files).

Noticed some issues :

  1. I expect to be able to click and drag the bottom bar to be able to zoom but doesn't seem to happen

  2. On Chrome (Version 72.0.3626.109 64-bit Linux) zooming doesn't perform well, I thought it didn't work at all at first

  3. On Chrome when trying to download SVGs of the ulx3s it doesn't work due to this Error: <circle> attribute r: Expected length, "NaN" (it seems to have some issue with a "drawing" layer which appears as huge circles as well).

  4. Maybe you want to style this scroll bar on the left -- it seems to be the default system style and could easily clash?
    image

  5. URL upload input should delete the default, i.e. be empty, when you focus on it

  6. URL upload doesn't seem to work for Kitspace, e.g. https://kitspace.org/boards/github.com/emard/ulx3s/ulx3s-6466905-gerbers.zip due to CORS :(, I probably need to enable it on the Kitspace server but it will probably be a common issue, maybe a little server side proxy/downloader would be best.

@mcous
Copy link
Member Author

mcous commented Feb 21, 2019

Thanks @kasbah! Appreciate the testing. Gonna run through more of this when I get home tonight, but for now:

I expect to be able to click and drag the bottom bar to be able to zoom but doesn't seem to happen

Shoot, I completely forgot to implement that. Thanks for the heads up

On Chrome (Version 72.0.3626.109 64-bit Linux) zooming doesn't perform well, I thought it didn't work at all at first

I got a little clever with the wheel event throttling, so I'm sure I missed some edge cases. So far I've tested Chrome and Firefox with trackpad and mouse on macOS. Were you using a mouse or a trackpad on Chrome linux? I have both available so will dig into this, too.

On Chrome when trying to download SVGs of the ulx3s it doesn't work...

That's weird, I'm unable to reproduce in Chrome or Firefox on mac. Maybe a caching issue? Could you try clearing it and/or seeing if it shows up on the dev server? Will also try myself on my Linux vm.

Maybe you want to style this scroll bar on the left

Yeah, I thought about it but I'm also kinda loath to do browser-specific tweaks (scrollbar customization is webkit-only). Might look into it as a follow up though?

URL upload input should delete the default, i.e. be empty, when you focus on it

Yeah right now it's kinda this weird thing where I want to make it easy to "try an example". Maybe it makes more sense to have that as a separate link rather than take over the URL upload field.

CORS

For now I'd rather not deal with setting up any sort of backend infrastructure for this, but I think this definitely should be doing two things:

  • Actually communicate errors to the user
  • Collect some anonymous usage analytics to measure if this is something people run into often

If it does happen a lot in the wild, then a simple lambda proxy seems definitely worth setting up

typings/raf-throttle.d.ts Outdated Show resolved Hide resolved
typings/nanologger.d.ts Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
@mcous mcous added feature view Related to @tracespace/view labels Feb 22, 2019
@mcous mcous changed the base branch from feat-typedefs to next February 22, 2019 03:51
@codecov
Copy link

codecov bot commented Feb 22, 2019

Codecov Report

Merging #104 into next will decrease coverage by 0.36%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #104      +/-   ##
==========================================
- Coverage   88.96%   88.59%   -0.37%     
==========================================
  Files          54       55       +1     
  Lines        2174     2183       +9     
==========================================
  Hits         1934     1934              
- Misses        240      249       +9
Impacted Files Coverage Δ
apps/view/scripts/serve.js 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb6e8f9...504aaf9. Read the comment docs.

apps/view/package.json Outdated Show resolved Hide resolved
apps/view/src/BoardSettings/SettingsForm.tsx Outdated Show resolved Hide resolved
apps/view/src/BoardSettings/color.tsx Outdated Show resolved Hide resolved
apps/view/src/BoardSettings/color.tsx Outdated Show resolved Hide resolved
@mcous
Copy link
Member Author

mcous commented Feb 24, 2019

Alright @kasbah how would you feel if I merged this now? Regarding your feedback:

  1. Implemented: 8c6230f

  2. Chrome on Linux (on my machine) was occasionally creating wheel events with deltaY: -0. This threw off an overly naive Math.sign comparison in the zoom throttler. Fixed with 6807f41

  3. Wasn't able to reproduce this on any of my computers / VMs with Chrome and Firefox. Can hopefully address if we get a repro

  4. I added some really basic styling using the CSS scrollbars standard proposal properties. Only Firefox supports them, but also Chrome on Lniux doesn't seem to mess with the default scrollbars like Firefox does, so I think that's all I really feel like doing for now. 622bdd1

  5. I still like the ability to quickly load an example (see Viewer at http://viewer.tracespace.io/ should have a default design loaded viewer#18), but I did add a select-all-text-on-click to the URL input field to make it easier to wipe the field out with your own url

  6. Still don't feel like setting up any backend infrastructure to support servers not set up for CORS just yet, but I did implement a very simple error toast so at least you know something went wrong. Future improvements here would be actual user-actionable error messages based on known failure modes. 22c42e0

    2019-02-24 11 33 27

I just pushed everything above to staging. Todos listed in PR description (or maybe just the CI one) are still gating for the actual release I think

@kasbah
Copy link
Collaborator

kasbah commented Feb 25, 2019

I feel good about merging in general since it's all brand new! Just starting to read the source to pick up some tricks.

  1. Chrome on Linux (on my machine) was occasionally creating wheel events with deltaY: -0. This threw off an overly naive Math.sign comparison in the zoom throttler. Fixed with 6807f41
  2. Wasn't able to reproduce this on any of my computers / VMs with Chrome and Firefox. Can hopefully address if we get a repro

So I am still experiencing the zoom issue on Chrome with my Thinkpad x230 Touchpad on Ubuntu 16.04. On Firefox it's still fine, though it does feel a bit like it's "jumpy" on both actually since there only seem to be a few zoom positions to select from. Is there a reason for that?

@mcous
Copy link
Member Author

mcous commented Feb 25, 2019

So I am still experiencing the zoom issue on Chrome with my Thinkpad x230 Touchpad on Ubuntu 16.04

Shoot. I've just pushed another change to the zoom throttling to this branch and staging. I'll also try to spin up a Ubuntu 16.04 VM (My machines are 18.04 currently) to test

it does feel a bit like it's "jumpy" on both actually since there only seem to be a few zoom positions to select from. Is there a reason for that?

Yeah still trying to optimize this. For a while with this I was trying to do a continuous zoom, but even on my relatively powerful machine I was finding the "continuous" experience to be really janky. I think this speaks to a need to really concentrate on simplifying the SVG renders themselves so the browser can do less work (e.g. #80), but for now...

The zoom is descretized into 17 ticks. This really simplifies some stuff (because zoom levels, both linear and logarithmic, can be pre-calculated), but I think that the zoom bar needs to do a better job of indicating that the zoom levels are discrete positions.

Longer term, I think a combination of increasing the number of ticks and also getting CSS transitions involved for the zoom itself will improve things, and possibly even trick the mind into thinking the zoom is continuous. At the moment though, the aforementioned render performance issues mean zoom transitions are also janky, so I left them out.

I figure discrete zoom levels is the least bad option at the moment 🙃

Copy link
Member Author

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Some final cleanup work before merge

@@ -0,0 +1,36 @@
# tracespace view troubleshooting
Copy link
Member Author

@mcous mcous Feb 25, 2019

Choose a reason for hiding this comment

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

@kasbah definitely want to hear your thoughts on this doc, since you've probably diagnosed more render problems than anyone else

(Also I think that it should eventually be a part of the site itself rather than a random markdown file)

apps/view/src/AppSettings/index.tsx Outdated Show resolved Hide resolved
apps/view/src/BoardDisplay/Controls.tsx Outdated Show resolved Hide resolved
apps/view/src/BoardDisplay/LayersRender.tsx Outdated Show resolved Hide resolved
apps/view/src/BoardSettings/ToggleOpenButton.tsx Outdated Show resolved Hide resolved
apps/view/src/images/types.d.ts Outdated Show resolved Hide resolved
apps/view/src/layers.ts Show resolved Hide resolved
apps/view/src/logger.ts Outdated Show resolved Hide resolved
apps/www/README.md Outdated Show resolved Hide resolved
typings/viewbox.d.ts Outdated Show resolved Hide resolved
@kasbah
Copy link
Collaborator

kasbah commented Feb 25, 2019

Maybe a future optimization could be to load the SVGs into WebGL (e.g. with three.js)? This could also allow for a 3d view (I noticed I keep wanting to spin the board around in the new tracespace viewer, as I do in the KiCad 3d viewer).

@mcous
Copy link
Member Author

mcous commented Mar 1, 2019

That's really cool! I haven't kept up with three.js, and I didn't know it could load SVGs now. Definitely down to add a 3D tab

@mcous mcous merged commit 4502adf into next Mar 6, 2019
@mcous mcous deleted the feat-add-viewer branch March 6, 2019 04:23
@kasbah kasbah mentioned this pull request Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature view Related to @tracespace/view
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants