-
-
Notifications
You must be signed in to change notification settings - Fork 350
feat: UI/UX enhancements for command result #2317
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
Conversation
Thanks for this - I haven't tested this out yet, but overall I like the idea. I do have some concerns though:
|
open={!!visibleCommandResult} | ||
onOk={() => setVisibleCommandResult(null)} | ||
onCancel={() => setVisibleCommandResult(null)} | ||
width={900} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This width exceeds the default width of the Inspector window. Responsive width could be a useful approach here: https://ant.design/components/modal#modal-demo-width
In the ideal case, the modal width would also shrink even further if the contents don't require all of it, but I'm not sure how possible this is. It's definitely more of a nice-to-have feature.
padding: 4px 4px 0 0; | ||
} | ||
|
||
.command-result-table :global(.ant-table-tbody) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, please use camelCase for CSS class names
import {copyToClipboard} from '../../../polyfills.js'; | ||
import styles from './Commands.module.css'; | ||
|
||
const isTimestampKey = (key) => key === 'timestamp'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how useful this conversion is, since it will only work for the timestamp
key, but the result may include timestamp values under another key. Rendering the raw value is perfectly fine, in my opinion.
}; | ||
|
||
return ( | ||
<Tooltip placement="topLeft" title={isCopied ? 'Copied' : 'Click to copy'}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These texts will need to have i18n support, so I would suggest simply reusing the Copied!
translation key from SelectedElement.jsx
. Click to copy
can be omitted if the cursor is changed accordingly.
const keys = Object.keys(data); | ||
if (keys.length === 1) { | ||
return createTableResult(data[keys[0]]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will result in the top-level key being hidden from the user
if (!tableData) { | ||
return <div>No data to display</div>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably won't be needed if the formatting option is disabled for primitive return values
columns={tableData.columns} | ||
pagination={false} | ||
size="small" | ||
scroll={{y: 400, x: 'max-content'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the y
value can be made responsive similarly to the width 🤔 I looked into this a few weeks ago, but couldn't find an answer right away.
Superseded by #2358 |
This is a suggestion for UI/UX enhancements of command result by rendering with a table. It's possible to copy values by clicking the table cell, and the columns can be sorted/filtered. Please try it out and let me know if the idea is good and how it can be improved.
Closes #2256, #2257