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

feat: add inline data inspector #338

Merged
merged 5 commits into from
Feb 18, 2022
Merged

feat: add inline data inspector #338

merged 5 commits into from
Feb 18, 2022

Conversation

connor4312
Copy link
Member

The old data inspector is preserved, but the new one is the default. It
functions (and looks) like the standard widget in the editor, and
basically just has information parity with the old sidebar inspector.

The old data inspector is preserved, but the new one is the default. It
functions (and looks) like the standard widget in the editor, and
basically just has information parity with the old sidebar inspector.

![](https://memes.peet.io/img/22-02-9bf5c4a2-4915-4e57-abda-dc9864bacdaa.png)
@lramos15
Copy link
Member

I would like to discuss this in the UX sync prior to merging it as I don't love an endianness checkbox and would like others thoughts on this

Copy link
Member

@lramos15 lramos15 left a comment

Choose a reason for hiding this comment

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

Left a few comments. I still worry about the maintainability of the hex editor given that this 500 lines of changes doesn't add any comments. Even just general "this clears the tooltip when you click off" or "reveals the data inspector" would be greatly appreciated. Also wonder if we should add some tests for the data inspector given that it can now be in 3 locations we should make sure it works the same in all the various locations. Thanks for the change :)!

.vscode/launch.json Outdated Show resolved Hide resolved
media/editor/dataInspectorProperties.tsx Show resolved Hide resolved
media/editor/vscodeUi.tsx Outdated Show resolved Hide resolved
Copy link
Member

@lramos15 lramos15 left a comment

Choose a reason for hiding this comment

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

We still have some code from the sidebar data inspector like the byte conversion that seems duplicated now

Copy link
Member

@lramos15 lramos15 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@lramos15 lramos15 merged commit 0936680 into main Feb 18, 2022
@lramos15 lramos15 deleted the feat/inline-data-inspector branch February 18, 2022 18:11
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.

2 participants