open-physiology / open-physiology-viewer

Apache License 2.0
7 stars 7 forks source link

Feature/75 - Ucsdapinat 75 - Demo mode as query parameter option , loader between states, improved tooltip for smaller lyphs. Opens settings panel by default when loading a model. #265

Closed jrmartin closed 1 year ago

albatros13 commented 1 year ago

Dear Jesus,

A couple of comments about the merge

  1. I could not checkout initially this branch because git at Windows does not like these paths, it is fixable with 'git config core.protectNTFS false', but it is not recommended: "This will not allow you to write files with special characters like a :. This will only allow people to write into your .git directory by using NTFS-specific hacks. This opens you up to attacks and does not solve the problem. "

Hence, my first suggestion is to remove ':' from the file names

08:17 Couldn't checkout origin/feature/75_cleanup invalid path 'test/snapshot_tests/snapshots/KeastSpinal_ConnectivityModel_Elements.test/Keast Spinal model group: Axon.png' invalid path 'test/snapshot_tests/snapshots/KeastSpinal_ConnectivityModel_Elements.test/Keast Spinal model group: Dendrite.png' invalid path 'test/snapshot_tests/snapshots/KeastSpinal_ConnectivityModel_Elements.test/Keast Spinal model group: Sympathetic chain.png'

  1. settingsPanel.js contains a lot of NeuroView specific code in toggleGroup. Can we move this code to a better place? SettingsPanel should remain minimal, it emits events when a control is changed and displays given data. It already grew too big and ideally should be split to more components (pretty much like many other bloated files).

  2. In the next sprint I also want to discuss with you some better organization for styles - we want the viewer to look uniform, I copied some control styling to materialEditor and other components to make it look similar to the main settings panel. The code now is very messy in this sense. We should perhaps create reusable .css files imported to components that need them.

Can you please fix issue #1, and we can work on #2 and #3 after merge.

Kind regards, Natallia

https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail Virus-free.www.avast.com https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

On Thu, 6 Jul 2023 at 02:30, Jesus M @.***> wrote:

Merged #265 https://github.com/open-physiology/open-physiology-viewer/pull/265 into develop.

— Reply to this email directly, view it on GitHub https://github.com/open-physiology/open-physiology-viewer/pull/265#event-9737838896, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKIRHHM34J7WPLWQCQSGX3XOYBIVANCNFSM6AAAAAAZ7UIPBM . You are receiving this because you are subscribed to this thread.Message ID: <open-physiology/open-physiology-viewer/pull/265/issue_event/9737838896@ github.com>