aframevr / aframe-inspector

:mag: Visual inspector tool for A-Frame. Hit *<ctrl> + <alt> + i* on any A-Frame scene.
https://aframe.io/aframe-inspector/examples/
MIT License
654 stars 201 forks source link

Fix 0 Toggle panels shortcut not properly working #648

Closed vincentfretin closed 1 year ago

vincentfretin commented 1 year ago

When disabling the editor and enabling it again via the Back to scene followed by Inspect scene button, the 0 Toggle panels shortcut wasn't properly working one time over two. This was because the listeners was added a second time, a third time, forth time... so removing the listeners in disable() wasn't working properly.

this.onKeyDown, this.onKeyUp are already bound in init. The function given to addEventListener in enable() and removeEventListener in disable() needs to be the same.

vincentfretin commented 1 year ago

Some people may not know this, you have a help modal when pressing the "h" key.

You can hide the left and right panels by pressing the 0 key. You will then have a "+" button on the center left.

Capture d’écran du 2022-10-10 15-28-07

vincentfretin commented 1 year ago

There is no reason to call forceUpdate after using setState here. https://reactjs.org/docs/react-component.html#forceupdate

The feature of showing again the panels is a little bit broken. First issue, the "+" should disappear when you click on Back to scene otherwise you click on the "+" and it does nothing. Second issue, there is actually two "+" buttons that are overlapping here, one for the left panel, and one for the right panel. The "+" for the right panel has the "right" class on it, so having it to the right was the intended use case here, but it seems the "right" style was lost.

vincentfretin commented 1 year ago

There was a third issue where clicking the "+" for a panel reset the visibility to false (undefined really) of the other panel. Now I think the feature is working as intended.

vincentfretin commented 1 year ago

Oh, the "+" buttons actually never show up if you test with npm run start from this repo. I saw them when I was testing with my site that includes tailwind reset styles. I need to add top:0 for them to show up in this repo.

vincentfretin commented 1 year ago

This PR can actually be merged only after we update the babel stack that I did in #641 because I used the spread syntax here

ERROR in ./src/components/Main.js
Module build failed: SyntaxError: Unexpected token (53:12)

  51 |         this.setState(prevState => ({
  52 |           visible: {
> 53 |             ...prevState.visible,
     |             ^
  54 |             attributes: !prevState.visible.attributes
  55 |           }
  56 |         }));

but you can review it already.

vincentfretin commented 1 year ago

The commit https://github.com/aframevr/aframe-inspector/pull/648/commits/a00448b49608a419c3f061851a1de4d1199e7763 also fixes issues with the delete modal. The issues was the following: When you delete with del/backspace, and you want to cancel, you need to click twice cancel. If you click ok, it will ask you right away if you want to delete another element next to the one you removed.

vincentfretin commented 1 year ago

I replaced the spread syntax by Object.assign so you can merge before we update the babel stack. If you want to test all the fixes from this PR and #638 I did a fix-shortcuts-listener-fix-some-ui-issues-dist branch with a build

<a-scene inspector="url:https://cdn.jsdelivr.net/gh/vincentfretin/aframe-inspector@8c45089755c9325b90e6f402cd9024b7c634f09a/dist/aframe-inspector.min.js"
vincentfretin commented 1 year ago

@dmarcos can you please merge those changes? If you are willing, you can give me a contributor role here so I can merge only the bug fixes, not new features. There are @kylebakerio and @kfarr who are interested by those fixes as well, and also #639. I have models library with drag n drop and json import/export on my roadmap, so I will start a community fork, merge all the changes I did here first and start the new features. It will be unfortunate that the community fork have the bugfixes and not the upstream version.

kfarr commented 1 year ago

I approve this message! To add more detail, I am trying to make updates to the 3dstreet editor that match close enough with @vincentfretin that we can also push improvements back to this repo. I am trusting that Vincent can help guide us through improving the inspector for all while also enabling neat community modules that we're all working on and would like to share

vincentfretin commented 1 year ago

@dmarcos can you please merge this one. You definitely want this bugfixes for aframe 1.4.0 release. Once you merge this one, I'll rebase #647 and finish it.

kylebakerio commented 1 year ago

@dmarcos the inspector is one of the buggiest problem areas of a-frame, it would be really great to have these fixes in and to encourage the development of this side of the project!