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

Fix 0 Toggle panels shortcut not properly working #648

Merged
merged 7 commits into from
Dec 6, 2022

Conversation

vincentfretin
Copy link
Contributor

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 vincentfretin mentioned this pull request Oct 10, 2022
@vincentfretin
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

left 0

.toggle-sidebar.right
right 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous rule matched element with "right" class as a child of element having "toggle-sidebar" class. This is not what we want here. The div has both the "toggle-sidebar" and "right" classes, so we want a ".toggle-sidebar.right" rule without space. If you put a space, it means the same as the previous rule.

@vincentfretin
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

The commit a00448b 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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

kfarr commented Nov 6, 2022

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
Copy link
Contributor Author

@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
Copy link

@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!

@dmarcos dmarcos merged commit a6d59bc into aframevr:master Dec 6, 2022
@vincentfretin vincentfretin deleted the fix-shortcuts-listener branch December 6, 2022 16:15
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.

None yet

4 participants