-
Notifications
You must be signed in to change notification settings - Fork 504
[CB] Remove tooltips from context menu #4033
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
base: devel
Are you sure you want to change the base?
Conversation
6dd1f10 to
285b6b7
Compare
|
|
||
| const title = translate(label); | ||
| const textRef = useRef<HTMLDivElement | null>(null); | ||
| const truncatedLabel = useTruncatedTooltip(textRef, label); |
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 think we dont need this, tooltips are just fine
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 also think so, we don't expect to have long tooltips or labels in the menu items, where we expect them, we should truncate them at the declaration point
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 example like here:
Lines 405 to 434 in a836412
| if (this.clipboardService.state === 'granted') { | |
| const filters = supportedOperations | |
| .filter(operation => !nullOperationsFilter(operation)) | |
| .map(operation => { | |
| const val = this.clipboardService.clipboardValue || ''; | |
| const wrappedValue = wrapOperationArgument(operation.id, val); | |
| const clippedValue = replaceMiddle(wrappedValue, ' ... ', 8, 30); | |
| const label = `${columnLabel} ${operation.expression} ${clippedValue}`; | |
| return new MenuBaseItem( | |
| { id: operation.id, icon: 'filter-clipboard', label }, | |
| { | |
| onSelect: async () => { | |
| const wrappedValue = wrapOperationArgument(operation.id, val); | |
| await this.applyFilter( | |
| model as unknown as IDatabaseDataModel<ResultSetDataSource>, | |
| resultIndex, | |
| key.column, | |
| operation.id, | |
| wrappedValue, | |
| ); | |
| }, | |
| }, | |
| { isDisabled: () => model.isLoading() }, | |
| ); | |
| }); | |
| result.push(...filters); | |
| } |
|
|
||
| const title = translate(label); | ||
| const textRef = useRef<HTMLDivElement | null>(null); | ||
| const truncatedLabel = useTruncatedTooltip(textRef, label); |
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 also think so, we don't expect to have long tooltips or labels in the menu items, where we expect them, we should truncate them at the declaration point
|
|
||
| const title = translate(label); | ||
| const textRef = useRef<HTMLDivElement | null>(null); | ||
| const truncatedLabel = useTruncatedTooltip(textRef, label); |
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 example like here:
Lines 405 to 434 in a836412
| if (this.clipboardService.state === 'granted') { | |
| const filters = supportedOperations | |
| .filter(operation => !nullOperationsFilter(operation)) | |
| .map(operation => { | |
| const val = this.clipboardService.clipboardValue || ''; | |
| const wrappedValue = wrapOperationArgument(operation.id, val); | |
| const clippedValue = replaceMiddle(wrappedValue, ' ... ', 8, 30); | |
| const label = `${columnLabel} ${operation.expression} ${clippedValue}`; | |
| return new MenuBaseItem( | |
| { id: operation.id, icon: 'filter-clipboard', label }, | |
| { | |
| onSelect: async () => { | |
| const wrappedValue = wrapOperationArgument(operation.id, val); | |
| await this.applyFilter( | |
| model as unknown as IDatabaseDataModel<ResultSetDataSource>, | |
| resultIndex, | |
| key.column, | |
| operation.id, | |
| wrappedValue, | |
| ); | |
| }, | |
| }, | |
| { isDisabled: () => model.isLoading() }, | |
| ); | |
| }); | |
| result.push(...filters); | |
| } |
Co-authored-by: Alexey Potsetsuev <[email protected]>
| let binding: string | undefined; | ||
| if (item.action.binding !== null) { | ||
| binding = getBindingLabel(item.action.binding.binding); | ||
| binding = getBindingLabels(item.action.binding.binding); |
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.
probably we don't want to change this, shortcuts might take a lot of space we can't display all of them in the menu, so it was intentionally to display first one
| const label = localizationService.translate('data_grid_table_filter_delete_for_column', undefined, { column: resultColumn?.name ?? '' }); | ||
| const { clippedLabel, tooltip } = getMenuLabelClipped(label); |
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.
please clip only resultColumn?.name ?? '' not whole label
| const clippedValue = replaceMiddle(wrappedValue, ' ... ', 8, 30); | ||
| const fullLabel = `${columnLabel} ${operation.expression} ${wrappedValue}`; | ||
| const { clippedLabel, tooltip } = getMenuLabelClipped(fullLabel); | ||
|
|
||
| return new MenuBaseItem( | ||
| { | ||
| id: operation.id, | ||
| label: `${columnLabel} ${operation.expression} ${clippedValue}`, | ||
| label: clippedLabel, |
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.
you can revert this changes and use fullLabel as a tooltip
| const title = `${columnLabel} ${operation.expression}`; | ||
| const { clippedLabel, tooltip } = getMenuLabelClipped(title); |
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.
only columnLabel should be clipped (replaceMiddle or getMenuLabelClipped)
| const clippedValue = replaceMiddle(wrappedValue, ' ... ', 8, 30); | ||
| const label = `${columnLabel} ${operation.expression} ${clippedValue}`; | ||
| const label = `${columnLabel} ${operation.expression} ${wrappedValue}`; | ||
| const { clippedLabel, tooltip } = getMenuLabelClipped(label); |
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.
please revert
| const nodeName = node?.name || ''; | ||
| const fullLabel = this.localizationService.translate('plugin_navigation_tree_filters_configuration', undefined, { name: nodeName }); | ||
| const isLongNodeName = nodeName?.length > 10; | ||
| const label = `${isLongNodeName ? fullLabel.replace(nodeName, nodeName.slice(0, 3) + '...') : fullLabel}...`; | ||
| const tooltip = isLongNodeName ? fullLabel : undefined; |
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.
only nodeName should be clipped (replaceMiddle or getMenuLabelClipped)
| }); | ||
| }, | ||
| ...actions.map(action => { | ||
| const { clippedLabel, tooltip } = getMenuLabelClipped(action.label); |
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.
probably we don't need to clip this values, they should not be long
closes https://github.com/dbeaver/pro/issues/7772