search

archimatetool / archi

Archi: ArchiMate Modelling Tool
https://www.archimatetool.com
MIT License
944 stars 269 forks source link

Zoom slider bug fix #788

Closed eduardobbs closed 2 years ago

eduardobbs commented 2 years ago

It fixes the problem reported by @YoeriVD Now using window.load event instead

Phillipus commented 2 years ago

Thanks, but this seems to fix it:

https://github.com/archimatetool/archi/commit/671384d79d2f4ab448133d6fa4452a0553a9a261

Phillipus commented 2 years ago

@eduardobbs Do you think the fix in 671384d is sufficient? Or is your method better?

@YoeriVD did you try it?

eduardobbs commented 2 years ago

@Phillipus, you put not only the zoom slider initialisation into the window.onload event, but a few other bits too (deep linking, height adjustments, etc.) From my experience, this may cause unexpected delays (small ones) and flickering (i.e. layout shift after the initial content paint) on some page elements, which can eventually affect user experience a bit, similar to some websites whereby you see some initial content, but before your finger clicks on a button, the button changes position, and you end up clicking on something else you did not mean to. If the network connection is fast and CPU is good/free, you may not even notice this flickering, but it's there. I haven't tested it myself in this particular case, but I would consider it a best practice anyway to put only the zoom slider init in the load event. You will probably save some testing effort and some poor user experience, and rework by doing so.

Phillipus commented 2 years ago

@eduardobbs Yes, I agree that it may have unwanted side-effects. Perhaps it would be better to merge your PR.

Phillipus commented 2 years ago

Merged. Thanks for the fix, @eduardobbs !