Project-Pandora-Game / pandora

https://project-pandora.com
Other
11 stars 3 forks source link

[ADD] a shortcut from the device context menu to the inventory view #715

Closed purpleharmony closed 4 months ago

purpleharmony commented 4 months ago

This adds a shortcut from room devices' context menus to their menus in the room inventory view.

It is fully working as far as I know, but this is my first contribution to pandora and first time working with react, so there are a few design decisions and technical points I'm unsure of and I'd like to get opinions on:

Design decision:

Technical points:

ClaudiaMia commented 4 months ago

Hi!~

I tested it and it works very well. I really love it. This was badly missing!

One thing I noticed that I never noticed before: When you use the new button to jump to the room item view of the room inventory, the tab is weirdly cut off at the top. One can see that the text is not fully v-center, because 2 or 3 pixel lines are missing from the top, which is noticeable from the back button and the tab itself, being not fully round. I have no idea how this happened, and when you open the room inventory manually, it is not like this. But it is also a very minor thing.

About the design decision: I guess it may not be immediately obvious what this button does, but I feel it is simple, elegant and clear enough solution to me. A user will quickly learn what it is and does.

About technical points: For that we need Sekkmer or Jomshir to address these points, as this is not my speciality~

Thank you!~

purpleharmony commented 4 months ago

Good catch! The vertical shift seems to be happening because of the autoscroll to the device in the room inventory list - it doesn't happen if I comment out the scrolling. Right now I don't know how to fix it, I would have to look more into it. But maybe someone does?

purpleharmony commented 4 months ago

I figured it out (well, kinda). I think what was happening was that the canvas was overflowing from its container, because canvas are display: inline; by default, and when the scroll event happened it scrolled the canvas in its container as well, hiding part of the tab.

purpleharmony commented 4 months ago

Thanks for the review. I rebased on master.