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

New project page components #342

Open
wants to merge 62 commits into
base: master
Choose a base branch
from

Conversation

dhovart
Copy link
Contributor

@dhovart dhovart commented Jun 8, 2022

  • These changes fix #__ (github issue number if applicable).

  • npm run mocha-test ran with full success.

  • npm run mocha-test resulted in failure locally: some permissions tests did not pass locally, although the backend was not changed.

  • All BrainBox tools behave as expected:

    • KEYS
      • right and left arrow keys
        • jump to next or previous slice within one brain, respectively
        • update the slider accordingly
        • update the slice number accordingly (upper left corner of the viewer window)
      • down and up arrow keys
        • jump to the next or previous brain within one project, respectively
        • update the selected subject in the annotation table
    • TOOL BUTTONS
      • minus
        • jumps to the previous slice within one brain
        • updates the slider accordingly
        • updates the slice number accordingly (upper left corner of the viewer window)
      • plus
        • jumps to the next slice within one brain
        • updates the slider accordingly
        • updates the slice number accordingly (upper left corner of the viewer window)
      • slider
        • updates slice view and slice number on the fly
      • sag / cor / axi buttons
        • switch view between the three orthogonal planes
      • show tool
        • when you click and drag in your browser window, this tool displays a cirlce at the position of your mouse click & drag as well as the user name in all browser windows connected to the same brain
      • the numbers at the bottom of the tool panel
        • change pencil size and eraser size accordingly
      • pencil tool
        • draws a line in the colour displayed in the color field
        • in combination with bucket tool filles a complete area with the chosen colour (be sure to have closed the contour line ;) Otherwise, the undo button will be your friend ;)
        • updates length and volume information (of what has been segmented) in the upper left corner of the viewer
      • erase tool
        • erases upon click drag from the annotation
        • in combination with the bucket tool erases the complete area of the color where you click
      • fill bucket tool
        • in combination with pencil tool
          • fills a complete area with the colour displayed in the colour field
        • in combination with erase tool
          • erases the complete area that is filled by the colour of where you click
      • colour field
        • displays the currently chosen colour to draw and fill
        • on click opens the set of colours available within the chosen label set where a new colour can be selected upon click
      • ruler tool
        • measures the distance between start and end of your defined path
          • points of mouse click appear and stay visible until you hit return key (this functionality is currently broken!) (you can click as many points as you wish to define the path you are interested in)
          • on return key, BrainBox will print the distance into the chat field (the ruler tool seems to be currently broken!!!)
      • adjust tool
        • slide opacity of overlaid annotation from 0 to 100%
        • increase or decrease brightness of the underlying MRI data
        • increase or decrease the contrast of the underlying MRI data
      • eyedropper tool
        • updates the colour field in the tool panel
        • displays/updates the region name in the upper left corner of the viewer
      • undo tool
        • undoes the user's actions in reverse chronological order and currently has the bug that it even undoes actions in slices you are currently not seeing! and there is currently no redo...
      • save button
        • saves the annotation of the data set into the data base
        • displays a message that user needs to login in case they are not
        • display a message Atlas saved Wed Oct 18 2017 12:49:12 GMT+0200 (CEST)

@dhovart dhovart force-pushed the new-project-page-components branch from e728e33 to 39d38fb Compare June 8, 2022 14:48
@dhovart
Copy link
Contributor Author

dhovart commented Jun 8, 2022

Hello, thanks for not merging this right now, I still have some more things to do as for instance removing the dependency on jQuery in the Atlasmaker files.
But feel free to test, review and comment the suggested changes here.

@dhovart dhovart force-pushed the new-project-page-components branch from dd6104e to 013e252 Compare June 19, 2022 16:52
@codecov-commenter
Copy link

codecov-commenter commented Jun 19, 2022

Codecov Report

Merging #342 (15f3809) into master (0a4bfba) will decrease coverage by 19.00%.
The diff coverage is 20.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff             @@
##           master     #342       +/-   ##
===========================================
- Coverage   64.33%   45.34%   -19.00%     
===========================================
  Files          16       16               
  Lines        2316     2329       +13     
===========================================
- Hits         1490     1056      -434     
- Misses        826     1273      +447     
Flag Coverage Δ
integration ?
unittests 45.34% <20.00%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
controller/mri/mri.controller.js 59.92% <0.00%> (-20.29%) ⬇️
controller/project/project.controller.js 72.56% <15.38%> (-16.97%) ⬇️
controller/project/index.js 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

@r03ert0
Copy link
Member

r03ert0 commented Jun 26, 2022

hello @dhovart! It works very nicely! I'm checking it:

  • would it be possible to limit the toolbox window to be contained within the MRI image?
  • would it be possible to replicate the look of the current buttons in the toolbox? (the line width is smaller and I thing the buttons are rounder too)
  • would it be possible to make the white drop shadow to look like the one in Microdraw which is grey?
  • in /mri the full screen button is not working (it does work in /project, however the toggle button doesn't appear selected)
  • when clicking the "save" button, the message is in a single line, which makes the toolbox window become momentarily large. Would it be possible to keep the width fixed?
  • the chat window is very short (which was the case already). Would it be possible to add a drag corner so that it can be made longer?
  • the toolbox is 2 columns (which happens when the window is large). Would it be possible to make it 1 column? Like this:

Screenshot 2022-06-26 at 17 43 44

or this (in Microdraw):

Screenshot 2022-06-26 at 17 44 21

- [x] when the toolbox is collapsed into a hamburger menu, the circle with the 3 horizontal lines stays in the place where the "x" was. Would it be possible to send it either to the left or to the right superior corner of the MRI image?

@dhovart
Copy link
Contributor Author

dhovart commented Jun 26, 2022

Hello! To make sure I'm understanding your requests:

  • would it be possible to limit the toolbox window to be contained within the MRI image?

The toolbox should still be a floating palette, but one cannot drag it outside of the MRI image, is that right?
For instance, we wouldn't be able to put it over the annotations pane.

  • the full screen button is not working

will correct that, it indeed doesn't on the MRI page

  • the chat window is very short (which was the case already). Would it be possible to add a drag corner so that it can be made longer?

So you'd want this on the chat scrollable window? Should the palette be resizable as well?

  • when the toolbox is collapsed into a hamburger menu, the circle with the 3 horizontal lines stays in the place where the "x" was. Would it be possible to send it either to the left or to the right superior corner of the MRI image?

ok, which corner is preferable to you?

@r03ert0
Copy link
Member

r03ert0 commented Jun 26, 2022

The toolbox should still be a floating palette, but one cannot drag it outside of the MRI image, is that right? For instance, we wouldn't be able to put it over the annotations pane.

Yes!

So you'd want this on the chat scrollable window? Should the palette be resizable as well?

Yes!

ok, which corner is preferable to you?
Could it be the one that's closer? If the pane is more to the left of the MRI image, the toolbox goes to the top left corner, and to the right top corner if it's not.

@dhovart
Copy link
Contributor Author

dhovart commented Jul 2, 2022

Looks like there are a few oddities with the resizable tool palette, will look into it.

@r03ert0
Copy link
Member

r03ert0 commented Jul 2, 2022

hey @dhovart, it's looking really nice!

  • would it be possible to indicate that the toolbox is resizable with a small icon like this?

Screenshot 2022-07-02 at 17 13 10

- [ ] when the toolbox is made taller, the chat div expands accordingly. However, if the chat is collapsed, the toolbox stays tall and displays a large empty space at the bottom: initial state:

Screenshot 2022-07-02 at 17 15 00

observed state after collapsing the chat:

Screenshot 2022-07-02 at 17 15 08

(continues...)

@r03ert0
Copy link
Member

r03ert0 commented Jul 2, 2022

(...continuation)
expected state after collapsing the chat:
expected

Screenshot 2022-07-02 at 17 23 12

- [x] In mri page (here http://localhost:3001/mri?url=...), don't bother separating the toolbox and putting it at the bottom. Just keep it over the MRI, as in project page. That will make our lives simpler for Microdraw. ![Screenshot 2022-07-02 at 17 24 06](https://user-images.githubusercontent.com/2310732/177006699-cdae1063-0628-4bd8-9988-5527cf612c88.png) - [ ] The fullscreen button doesn't work in MRI page (it works fine in project page). - [x] The "script" space doesn't resize with the toolbox (unlike the "chat" space which resizes all fine)

Screenshot 2022-07-02 at 17 27 58

(continues...)

@r03ert0
Copy link
Member

r03ert0 commented Jul 2, 2022

(...continuation)

  • When a message is displayed, the "chat" space overflows the toolbox. Expected: the whole toolbox should momentarily expand and then shrink.
    overflow

@dhovart
Copy link
Contributor Author

dhovart commented Jul 2, 2022

All right, will look into this, thanks for the feedback.

@dhovart dhovart force-pushed the new-project-page-components branch 2 times, most recently from 19dc9f9 to 8752e98 Compare August 9, 2022 15:05
@r03ert0
Copy link
Member

r03ert0 commented Aug 9, 2022

thank you for the commits! I updated the feedback items to show as checkboxes, and marked [x] the resolved ones :)

@dhovart
Copy link
Contributor Author

dhovart commented Aug 9, 2022

All right, thank you. Should we disable vertically resizing when the chat or script is collapsed ?

@dhovart
Copy link
Contributor Author

dhovart commented Aug 9, 2022

Also, I can't reproduce the issue where the file name is not appearing...

@dhovart
Copy link
Contributor Author

dhovart commented Aug 9, 2022

" When a message is displayed, the "chat" space overflows the toolbox" -> this one is not fixed for you ?

@r03ert0
Copy link
Member

r03ert0 commented Aug 9, 2022

  • Text annotations do not update, unless there is a 2nd user connected (in which case they are very nicely updated in real time). So, if there's only one subject, and I write text in "Comments", for example, and then reload, nothing will have been saved.

@dhovart dhovart force-pushed the new-project-page-components branch from ccf7e33 to 9f0a440 Compare August 10, 2022 16:14
@dhovart dhovart force-pushed the new-project-page-components branch from 2b22f70 to b15ac6b Compare September 26, 2022 15:43
@r03ert0
Copy link
Member

r03ert0 commented Nov 14, 2022

hi @dhovart! I'm checking the new PR.
After npm install, npm run build, npm start, I open localhost:3001, select the "Lion from Zenodo" from the list of brains, and nothing displays. I'm attaching screenshots of the display and the console.

Screenshot 2022-11-14 at 16 09 10

Screenshot 2022-11-14 at 16 09 21

@dhovart
Copy link
Contributor Author

dhovart commented Nov 15, 2022 via email

@r03ert0
Copy link
Member

r03ert0 commented Nov 23, 2022 via email

@dhovart
Copy link
Contributor Author

dhovart commented Jan 5, 2023

Hi, sorry, the published package on npm wasn't matching the sources and there was indeed an import missing. I've bumped it to 1.2.1 and it seems to be working now, so it should be ok on your side as well.

@r03ert0
Copy link
Member

r03ert0 commented Jan 5, 2023 via email

@katjaq katjaq requested a review from ntraut January 30, 2023 20:16
@ntraut
Copy link
Member

ntraut commented Feb 1, 2023

Hello @dhovart, I recently updated brainbox dependencies and performed some linting, which produced some conflicts, sorry for that.
I can merge the master branch to yours if you want or rebase your branch, which one do you prefer?

@r03ert0
Copy link
Member

r03ert0 commented Feb 7, 2023

The fullscreen button doesn't work on an ipad (i still see the table). It does work on desktop.

@r03ert0
Copy link
Member

r03ert0 commented Feb 7, 2023

I can't drag the toolbar in an ipad (maybe the toolbar is only using click but not touch events?)

@dhovart dhovart force-pushed the new-project-page-components branch from 271f6fc to 19fb533 Compare December 29, 2023 11:06
@dhovart dhovart force-pushed the new-project-page-components branch from 4495c57 to 92cf6d1 Compare January 28, 2024 19:57
Copy link
Member

@katjaq katjaq left a comment

Choose a reason for hiding this comment

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

Thanks so much for moving this forward!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants