-
Notifications
You must be signed in to change notification settings - Fork 55
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
Print export progress, button disabling and slight Component refactor #1529
Conversation
This makes sense as can't think of a scenario where a partially loaded map should be printed. What does everyone else think, is it clear enough to disable the buttons? It's often noticable that tiles are loading / the preview is working, though maybe not super obvious for the OSM map / if the paper size is large and the tiles are happening off-screen. It doesn't take much to make the mental connection though.
The issue specifies pdf and this PR solves pdf yet it seems reasonable to do the same for Save image. |
212b0c2
to
ad20a6e
Compare
@Grammostola Good point, I've added the loading indicator to PNG export as well now. |
#1515 Also introduces a spinner utility. It was previously only used internally in infowindow when exporting selection. It is however used inline, not as a global DOM-object. Also that one is an oldschool animated gif so they doesn't look the same. Not sure which is the best approach to have a global spinner or an inline. Maybe there is a place for both, but code wise it should be implemented in the same utility module so it is easier to find when developing and use the same look. In terms of technology, I prefer a css-spinner over gif, but I was lazy and didn't change the old one. Btw, there already is (at least) one hidden class: |
Having the same spinner would make sense, if nothing else then for consistency across the UI. Global vs. local usually depends on how much of the UI should be locked down; while printing we can't allow the user to click on anything since it might interfere with the process. I can adjust the usage of the gif-spinner to also use the same CSS, do you want me to do so as part of this PR?
I saw that one, but it was scoped to |
@sweco-sedalh Totally agree that global vs inline depends on if the user can do something else in the meantime but also the visual impact and timeframe, so probably there is a place for both. Just wanted to raise the question as I haven't seen this one in action (just had a look at the code). As #1515 is not yet merged I think it's easier to just leave it for now as they would not really benefit from each another anyway. Didn't realise the spinner was outside the o-map scope, but still I think it quickly becomes confusing when having several different classes doing the same thing. But on the other hand: I don't know anything about css design. |
ad20a6e
to
7018384
Compare
LGTM |
Component
to make use of ES6 class syntax as well as introduce a conveniencehtml
tagged template literal for use inrender()
methodsThe third point is introduced in a way so that it does not affect its interface. If wanted it can be extracted into a separate PR.